Skip to content

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.


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.

}

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.


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.

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