-
Notifications
You must be signed in to change notification settings - Fork 788
[NOT FOR COMMIT] First try at getting unions supported, does type checking but does not #2270
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
Conversation
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.
Some quick comments.
@@ -973,6 +1023,8 @@ class SyclKernelFieldHandler { | |||
SyclKernelFieldHandler(Sema &S) : SemaRef(S) {} | |||
|
|||
public: | |||
|
|||
static const constexpr bool VisitUnionBody = false; |
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.
Default opt-out here.
@@ -1093,6 +1148,8 @@ class SyclKernelFieldChecker : public SyclKernelFieldHandler { | |||
} | |||
|
|||
public: | |||
static const constexpr bool VisitUnionBody = true; |
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.
This is how a handler opts-into this.
@@ -1108,13 +1165,39 @@ class SyclKernelFieldChecker : public SyclKernelFieldHandler { | |||
return isValid(); | |||
} | |||
|
|||
bool handlePointerType(FieldDecl *FD, QualType FieldTy) final { | |||
// TODO: Replace with a better diagnostic. | |||
if (UnionCount) { |
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.
I considered making this a separate handler that only overloaded enter/leave Union and the handleSyclAccessorType/handleSyclPointerType.
We might also consider still doing this, and putting it in after the normal FieldChecker. Note the bad error messages.
@@ -849,6 +856,41 @@ class KernelObjVisitor { | |||
void VisitRecord(const CXXRecordDecl *Owner, ParentTy &Parent, | |||
const CXXRecordDecl *Wrapper, Handlers &... handlers); | |||
|
|||
// Base case, only calls these when filtered. |
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.
This 'VisitUnion' function is the main trickery. See this to play around with it: (https://godbolt.org/z/z8b61d) .
There are 3 versions of the function. This first is the 'base' case.
The list of 'handlers' not specified via <> syntax (as we do when they aren't specified) are deduced like normal. This ends up in the normal 'Handlers' type pack, with the first of that pack being picked up by the CurHandler.
the return type is done with std::enable_if, which switches based on whether we should VisitUnionBody for the 'first' handler (the one in CurHandler).
If we SHOULD visit it, we call VisitUnion again, this time with the CurHandler put into the "FilteredHandlers" pack (which will never be deduced, since it is not the 'last' thing.
If we SHOULDN'T visit it, we call VIsitUnion again, this time with the CurHandler discarded from the list, and forward on the"FilteredHandlers" (which need to be specified inside <...>, since they cannot be deduced).
If there are no more items to be put into the cur_handler type (or deduced as a Handlers type), the base is called, which does the work.
// TODO: What other types do we need ot check? What types other than | ||
// pointer/accessor requires a decomposition? |
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.
We need to check sampler and stream types. Union does not work with them.
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.
Cool, thanks! I'm also considering breaking the union checking into a separate handler, so it would need to handle all of these.
Mention explicitly in the README that only SPIR-V modules that declare the Kernel capability are supported. This is a key restriction for anyone looking to translate between (arbitrary) SPIR-V and LLVM IR, so mention it at the top of the README. Original commit: KhronosGroup/SPIRV-LLVM-Translator@8e6f467
[L0 v2] Use single command list for all operations and implement deferred event deallocation
descend.
Here is my first run at this. I think it works, but I haven't written any tests for it yet. Additionally, it needs a good error message for the things inside a union. Additionally, I need to identify all the things that need decomposing.
Finally, our error messaging for 'inside a union' is ALSO probably something we want to do a 'trace-back' on. Something like a, "In this struct, in this struct, in this union, in this struct, from this param".
I believe we can do exactly the same type of thing for descending from a struct as well by changing VisitRecord. It would be broken up into 2 implementations, a VisitRecordFiltered (Almost identical to VisitUnion, except the base-case calls VisitRecordDecomposing), and VisitRecordDecomposing (which just does what VisitRecord doesnow!).
So VisitRecord's implementation would be:
Additionally, the enable_if_t would be based on whether the handler needs to be visited in that case.
EDIT TO ADD: I just realized, we can probably just make VisitRecord call the VIsitRecordFiltered (by the way, the names I chose above are terrible...) 'base' instead! So it would be: