[PATCH] D103112: Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name

[Erich Keane via Phabricator via cfe-commits]

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