rjmccall added inline comments.
================
Comment at: clang/docs/LanguageExtensions.rst:2413
+(or type of the expression) that is stable across split compilations, mainly to
+support SYCL/Data Parallel C++ language.
+
----------------
The semantics here seem specific to SYCL. In fact, if this feature were used
in normal code, it would arguably be weird that it changed behavior
significantly in SYCL. I think we should just acknowledge that and make it a
feature that's only enabled when SYCL is enabled. That probably also means
renaming it to `__builtin_sycl_unique_stable_name`.
================
Comment at: clang/include/clang/AST/Expr.h:2045
+// representation of the type (or type of the expression) in a way that permits
+// us to properly encode information about the SYCL kernels.
+class UniqueStableNameExpr final
----------------
Since this is really just for internal use in system headers (right?), is there
a need for it to be as flexible as this about whether it takes an expression or
a type?
================
Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1965
+ // UniqueStableNameExpr
+ EXPR_UNIQUESTABLENAME,
};
----------------
UniQuestableName :)
Probably ought to use underscores here.
================
Comment at: clang/lib/AST/ASTContext.cpp:11662
+
+void ASTContext::AddSYCLKernelNamingDecl(const CXXRecordDecl *RD) {
+ RD = RD->getCanonicalDecl();
----------------
Can we assert that these methods are only called in SYCL? I'd hate to
accidentally do this bookkeeping in other modes.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1977
+ if (Context.getShouldCallKernelCallback()(Context.getASTContext(), Lambda)) {
+ Context.getKernelMangleCallback()(Context.getASTContext(), Lambda, Out);
+ Out << '_';
----------------
This basically assumes that the callback is only changing the discriminator.
And in fact, we already have this "device lambda mangling number" concept that
we use in different modes with similar problems to SYCL. Can we unify these
and just provide one way for the context to opt in to overriding discriminators?
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:5056
+
+ Out << "u20__unique_stable_name";
+ if (USN->isExpr()) {
----------------
The expectation is that these names match the spelling in the source, so this
should include the `__builtin` prefix.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103112/new/
https://reviews.llvm.org/D103112
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
