Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

erichkeane
Copy link
Contributor

@erichkeane erichkeane commented Aug 5, 2020

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:

if (shouldDecompose(Wrapper)) {
  VisitRecordDecomposing(Owner, Parent, Wrapper, handlers...);
} else {
 VisitRecordFiltered(Owner, Parent, Wrapper, handlers...);
}

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:

if (shouldDecompose(Wrapper)) {
  VisitRecordFiltered<Handlers...>(Owner, Parent, Wrapper, handlers...);
} else {
 VisitRecordFiltered(Owner, Parent, Wrapper, handlers...);
}

Copy link
Contributor Author

@erichkeane erichkeane left a 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;
Copy link
Contributor Author

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;
Copy link
Contributor Author

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) {
Copy link
Contributor Author

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.
Copy link
Contributor Author

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.

Comment on lines +1193 to +1194
// TODO: What other types do we need ot check? What types other than
// pointer/accessor requires a decomposition?
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@erichkeane erichkeane closed this Aug 11, 2020
jsji pushed a commit that referenced this pull request Dec 21, 2023
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
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
[L0 v2] Use single command list for all operations and implement deferred event deallocation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants