[SYCL] Add __builtin_num_fields and __builtin_field_type #3658
Conversation
This is an experimental branch to see whether we can implement the device copyability checking functionality within the SYCL library by using some builtins to provide super rudimentary introspection of structures and lambdas. These builtins will tell you how many fields are in the structure or how many captures are in the lambda, and tell you what types those fields are. __builtin_num_fields() accepts a type id and returns the number of fields in the given type as an integer constant expression. __builtin_field_type() accepts a type id and an integer constant expression and returns a "value" of the field at the given offset from the given type. The call should always be wrapped in a decltype (or similar) expression to get the actual type of the field. The value returned is meaningless and it is UB to inspect it. Note: if we wish to upstream any of this functionality, this builtin should be replaced by a new type in the type system, akin to how decltype and typeof work. More work is still needed for the SYCL library to fully support the checking in that there is a need to get the number of base classes and the base class type information.
This saves 8 bytes from the AST node.
NOTE: this has a fair amount of code duplication that should be cleaned up if we're happy with the experiment. The num_bases and num_fields AST nodes are basically the same thing and so those could be differentiated with a single bit in the AST node. A common base class might also be a reasonable approach.
|
The remaining clang-format issues are not ones I am willing to fix. The formatting in RecursiveASTVisitor matches the surrounding code and would make the code harder to read instead of easier. The formatting in ItaniumMangle is existing formatting in upstream and serves to make it easier to append additional cases with less diff marking. |
Returning Loc instead of making a call to do it Changed the names of some diagnostics to be more agnostic Changed the name of a parameter to be more bool-like Fixed up some assertions that were doing too much work
We don't need to store this information *twice* -- the expression type itself is already storing the same information, so just rely on that.
This allows experimentation with the new builtins.
AaronBallman
requested review from
elizabethandrews and
premanandrao
as code owners
|
#3794 validated the design, so I think this is now ready for review. |
|
Ping |
There was a problem hiding this comment.
I'm sorry for the delay. It took me a while to go through the patch. It looks ok to me barring a few nits. Having said that, I don't feel very qualified to review this and so I'm hoping @erichkeane and/or @premanandrao can also take a look.
There was a problem hiding this comment.
LGTM! @erichkeane and/or @premanandrao please confirm as well.
I have started looking at this one @AaronBallman, sorry for the delay. |
There was a problem hiding this comment.
@AaronBallman, I just have a couple nits. My review was mostly my trying to understand these changes, some in areas that I am not familiar with.
Also, clang-format does not seem to like the formatting in a couple of files.
| if (!Idx->isValueDependent()) { | ||
| Optional<llvm::APSInt> IdxVal = Idx->getIntegerConstantExpr(Context); | ||
| if (IdxVal) { | ||
| CXXRecordDecl *RD = SourceTy->getAsCXXRecordDecl(); |
Yup, clang-format can get over itself. :-D It's barking about following existing style patterns in the file, so I don't intend to fix those. |
AaronBallman
dismissed stale reviews from erichkeane and elizabethandrews
via
510357d
|
Build bot failures are unrelated to the changes in the patch: |
AaronBallman
dismissed stale reviews from premanandrao and elizabethandrews
via
3827ebc
The merge conflicts were from Erich's builtin unique stable name. It turns out that landing two different new builtins at the same time has some conflicts. ;-) Thankfully, they were all easy to resolve. |
This is an experimental branch to see whether we can implement the
device copyability checking functionality within the SYCL library by
using some builtins to provide super rudimentary introspection of
structures and lambdas. These builtins will tell you how many fields
are in the structure or how many captures are in the lambda, and tell
you what types those fields are.
__builtin_num_fields() accepts a type id and returns the number of
fields in the given type as an integer constant expression.
__builtin_field_type() accepts a type id and an integer constant
expression and returns a "value" of the field at the given offset from
the given type. The call should always be wrapped in a decltype (or
similar) expression to get the actual type of the field. The value
returned is meaningless and it is UB to inspect it. Note: if we wish to
upstream any of this functionality, this builtin should be replaced by
a new type in the type system, akin to how decltype and typeof work.
More work is still needed for the SYCL library to fully support the
checking in that there is a need to get the number of base classes and
the base class type information.