Skip to content

[SYCL-MLIR] Opaque pointer support Polygeist-to-LLVM #8767

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

sommerlukas
Copy link
Contributor

First step to add opaque pointer support for the SYCL-MLIR project: Make the lowering patterns in the Polygeist-to-LLVM lowering compatible with opaque pointers.

Partly resolves #8616.

@sommerlukas sommerlukas added the sycl-mlir Pull requests or issues for sycl-mlir branch label Mar 24, 2023
@sommerlukas sommerlukas requested a review from etiotto as a code owner March 24, 2023 16:57
@sommerlukas sommerlukas self-assigned this Mar 24, 2023
@sommerlukas
Copy link
Contributor Author

I did not want to migrate all our components to opaque pointers in a single step, as the resulting PR would be huge. Instead, this is the first step, affecting only the Polygeist to LLVM lowering.

As some of the patterns are shared with other lowerings and other tests/components still rely on this lowering to produce typed pointers, I had to introduce a "versioning". The existing patterns now have a "Old" suffix, and the patterns with the unmodified name have been changed to support opaque pointers. The polygeist-to-llvm pass additionally supports a use-opaque-pointers option (defaulting to false), so it can continue to produce typed pointers when other parts rely on that (e.g. cgeist).

The patterns (and functions) with the "Old" suffix as well as the option can be removed as soon as we complete the transition to opaque pointers. They are unmodified compared to sycl-mlir, so do not need to be reviewed.

Having the versioning and option also allow us to continue to produce typed pointers, in case other tools out-of-scope of this project (e.g., SPIR-V translator) still need to complete the transition.

The LIT tests for the Polygeist to LLVM lowering already verify the correctness of the opaque pointer version.

Copy link
Contributor

@victor-eds victor-eds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have the pointee type in all check lines with GEPOp operations?

@sommerlukas sommerlukas force-pushed the opaque-pointer-polygeist-to-llvm branch from ce865e5 to ee55e26 Compare March 28, 2023 08:06
@sommerlukas
Copy link
Contributor Author

I assume CI will currently failed, as I have rebased on top of #8774 and the branch is currently still missing the fix from #8817. Will need to rebase again once #8817 has been merged.

Signed-off-by: Lukas Sommer <lukas.sommer@codeplay.com>
Signed-off-by: Lukas Sommer <lukas.sommer@codeplay.com>
Signed-off-by: Lukas Sommer <lukas.sommer@codeplay.com>
Signed-off-by: Lukas Sommer <lukas.sommer@codeplay.com>
Signed-off-by: Lukas Sommer <lukas.sommer@codeplay.com>
Signed-off-by: Lukas Sommer <lukas.sommer@codeplay.com>
@sommerlukas sommerlukas force-pushed the opaque-pointer-polygeist-to-llvm branch from ee55e26 to f7823e0 Compare March 28, 2023 14:41
@sommerlukas
Copy link
Contributor Author

I assume CI will currently failed, as I have rebased on top of #8774 and the branch is currently still missing the fix from #8817. Will need to rebase again once #8817 has been merged.

Rebased now that #8817 has been merged.

@sommerlukas sommerlukas requested a review from etiotto March 28, 2023 14:41
@sommerlukas sommerlukas merged commit b1e03ed into intel:sycl-mlir Mar 28, 2023
@sommerlukas sommerlukas deleted the opaque-pointer-polygeist-to-llvm branch March 28, 2023 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sycl-mlir Pull requests or issues for sycl-mlir branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants