-
Notifications
You must be signed in to change notification settings - Fork 128
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
[CIR][CodeGen][LowerToLLVM] Emit OpenCL version metadata for SPIR-V target #773
Conversation
Other idea includes adding a new But I think our IR should be composed of language-version-agnostic constructs. Also, that would not conflict with this one. I prefer not early abstracting it. If we have more use cases for something like Btw, refactor of CodeGen logic specific to OpenCL will come right after this patch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on keeping this OpenCL-specific for now, and plan to abstract later on demand.
if (auto openclVersionAttr = mlir::dyn_cast<mlir::cir::OpenCLVersionAttr>( | ||
attribute.getValue())) { | ||
auto *int32Ty = llvm::IntegerType::get(llvmContext, 32); | ||
llvm::Metadata *olcVerElts[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be oclVerElts
.
Example: | ||
``` | ||
// Module compiled from OpenCL 1.2. | ||
module attributes {cir.cl.version = cir.cl.version<1,2>} {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super-Nit: The attribute is printed cir.cl.version<1, 2>
(with a space after the comma) according to the added lit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look overall good, minor improvement request!
Example: | ||
``` | ||
// Module compiled from OpenCL 1.2. | ||
module attributes {cir.cl.version = cir.cl.version<1, 2>} {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add an alias so that the attribute could be a little less verbose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually looks even more verbose with the alias! Hopefully we can have a better output once we implement our owne cir module. Anyways, please go back to the previous version (without the alias), I'll approve now and merge later when you update again, sorry the back-n-forth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with another nit
This reverts commit 7c8f56c.
Similar to #767, this PR emit the module level OpenCL version metadata following the OG CodeGen skeleton.
We use a full qualified
cir.cl.version
attribute on the module op to store the info in CIR.