rjmccall added a comment.
Thanks, that seems to work out cleanly.
================
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
----------------
erichkeane wrote:
> rjmccall wrote:
> > rjmccall wrote:
> > > 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?
> > That said, I don't really have a strong objection to supporting either a
> > type or an expression operand.
> I had responded to this I thought? I found no good reason to do expression,
> we can sprinkle decltype around to deal with that, I'll prep a patch to
> remove the expr bits.
Okay. So this doesn't really need trailing storage anymore, and the
documentation needs to be updated to not mention the expression production.
================
Comment at: clang/include/clang/AST/Mangle.h:174
+ using KernelMangleCallbackTy =
+ llvm::Optional<unsigned> (*)(ASTContext &, const CXXRecordDecl *);
explicit ItaniumMangleContext(ASTContext &C, DiagnosticsEngine &D)
----------------
The concept here isn't kernel-specific, and it's not an arbitrary callback. I
think it would be better to call this something more generic, like
DiscriminatorOverride.
Should the argument have to be a record? Other local declarations can show up
as e.g. template arguments, like enums or (I think) static local variables.
================
Comment at: clang/lib/AST/ASTContext.cpp:11689
+ assert((getLangOpts().SYCLIsDevice || getLangOpts().SYCLIsHost) &&
+ "Only valid for SYCL programs");
+ RD = RD->getCanonicalDecl();
----------------
Could you add an `isSYCL()` method on LangOpts as a convenience for this?
================
Comment at: clang/lib/Basic/IdentifierTable.cpp:159
return KS_Future;
+ if ((LangOpts.SYCLIsDevice || LangOpts.SYCLIsHost) && Flags & KEYSYCL)
+ return KS_Enabled;
----------------
I think people would generally happier if you parenthesized `Flags & KEYSYCL`,
even though, yes, this does work under C precedence rules.
================
Comment at: clang/lib/CodeGen/CGCUDANV.cpp:196
+ // If the host and device have different C++ ABIs, mark it as the device
+ // mangle context so that the mangling needs to retrieve the additonal
+ // device lambda mangling number instead of the regular host one.
----------------
typo: additonal
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
