erichkeane added inline comments.
================
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
----------------
rjmccall wrote:
> 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.
Oh! Good point! *doh!*
================
Comment at: clang/include/clang/AST/Mangle.h:174
+ using KernelMangleCallbackTy =
+ llvm::Optional<unsigned> (*)(ASTContext &, const CXXRecordDecl *);
explicit ItaniumMangleContext(ASTContext &C, DiagnosticsEngine &D)
----------------
rjmccall wrote:
> 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.
Currently we are only specializing this for lambdas, so I chose CXXRecordDecl
to avoid the need to cast in each of the 'discriminators'. I will switch it to
'NamedDecl', though both discriminators will be only checking CXXRecordDecl
stuff.
================
Comment at: clang/include/clang/Basic/LangOptions.h:448
+
+ bool isSYCL() const { return SYCLIsDevice || SYCLIsHost; }
};
----------------
aaron.ballman wrote:
> FWIW, we also have `SYCLVersion != SYCL_None` as a possible way to express
> this. Perhaps we should just use that anywhere we're using `SYCLIsDevice ||
> SYCLIsHost` currently? (I don't have a strong opinion but am bringing it up
> in case someone else does.)
IMO, that is a fairly 'indirect' mechanism for it. I think host+device is the
'right' way to do it, an d I think the SYCLVersion is a 'consequence' of those.
It is unfortunate that SYCL forces us to have 3 things to express in language
opts (vs 2 for the other languages).
================
Comment at: clang/test/CodeGenSYCL/unique_stable_name.cpp:86-89
+ // FIXME: Ensure that j is incremented because VLAs are terrible.
+ int j = 55;
+ puts(__builtin_sycl_unique_stable_name(int[++j]));
+ // CHECK: call spir_func void @puts(i8 addrspace(4)* addrspacecast (i8*
getelementptr inbounds ([[STRING_SIZE]], [[STRING_SIZE]]* @[[STRING]], i32 0,
i32 0) to i8 addrspace(4)*))
----------------
aaron.ballman wrote:
> @rjmccall -- any opinions or ideas on this? I think VLAs should likely
> behave the same as they do in `sizeof`, etc.
VLA side-effects are a bit of an edge case in the language that I think are an
unfortunate 'gotcha'.
I'm not sure where I fall on this in general (other than hating it as a
'feature' of C), but I'd hope that this is something we can leave as a FIXME,
as this is a builtin for the use of a library. I don't suspect that it will
affect the normal uses of this builtin at all, so I hope it is something we
look into once someone actually cares.
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
