[SYCL] Implement anonymous namespace spec-id functionality. #3576
Conversation
In order to get the specialization_id metadata to be usable in the integration footer, we need to insert a 'shim' function at each anonymous namespace so that we can do a lookup at the 'right' location, then look it up again later by the type. This implements all that functionality based on my latest understanding of the design.
erichkeane
requested review from
AaronBallman,
elizabethandrews and
premanandrao
as code owners
clang/lib/Sema/SemaSYCL.cpp
Outdated
| // Emit the list of shims required for a DeclContext, calls itself recursively. | ||
| static std::string EmitSpecIdShims(raw_ostream &O, unsigned &ShimCounter, | ||
| const DeclContext *DC, | ||
| std::string NameForLastShim) { |
There was a problem hiding this comment.
Can NameForLastShim be a reference?
There was a problem hiding this comment.
The intent was for it to be a 'local variable' here so that it could modify it without affecting the parent. That said, it seems that it only does tail-call, so I believe it can. We just need to make sure with the change that we never allow anything that modifies this variable to be below the recursion.
| constexpr sycl::specialization_id same_name{7}; | ||
| // CHECK-NEXT: namespace { | ||
| // CHECK-NEXT: namespace __sycl_detail { | ||
| // CHECK-NEXT: static constexpr decltype(same_name) &__spec_id_shim_[[SHIM_ID:[0-9]+]]() { |
There was a problem hiding this comment.
Is it worth it to test that this SHIM_ID and others below are a different value than the previous ones?
There was a problem hiding this comment.
I'd thought about that, but I'm not aware of a good way to make them all be wildcards, yet still make sure they are different. Unfortunately there doesn't seem to be any sort of mechanism in CHECK to do something like that.
Do you have a suggestion?
There was a problem hiding this comment.
Sorry, no, I don't have a suggestion. It was just a thought.
| constexpr sycl::specialization_id same_name{7}; | ||
| // CHECK-NEXT: namespace { | ||
| // CHECK-NEXT: namespace __sycl_detail { | ||
| // CHECK-NEXT: static constexpr decltype(same_name) &__spec_id_shim_[[SHIM_ID:[0-9]+]]() { |
There was a problem hiding this comment.
Sorry, no, I don't have a suggestion. It was just a thought.
| // CHECK-NEXT: namespace detail { | ||
| // CHECK-NEXT: template<> | ||
| // CHECK-NEXT: inline const char *get_spec_constant_symbolic_ID<::__sycl_detail::__spec_id_shim_[[SHIM_ID]]()>() { | ||
| // CHECK-NEXT: return ::__sycl_detail::__spec_id_shim_[[SHIM_ID]](); |
There was a problem hiding this comment.
It doesn't seem like this code will compile: we return a pointer to a specialization_id variable from a function which is expected to return const char *. symbolic_ID should be return there
| OS << "template<>\n"; | ||
| OS << "inline const char *get_spec_constant_symbolic_ID<" << TopShim | ||
| << ">() {\n"; | ||
| OS << " return " << TopShim << ";\n"; |
There was a problem hiding this comment.
I guess here we should have emitSpecIDName instead of TopShim, see comment below in test
There was a problem hiding this comment.
its not the spec-id name, because of course we cannot name that.
Sorry for such late review, but there seems to be a bug in the patch
Looks like I got carried away with the implementation and we didn't catch that I was returning the wrong things from the 'end call function' when generating anonymous function shims. This was brought up here: intel#3576 (review) This should fix that problem.
…3669) Looks like I got carried away with the implementation and we didn't catch that I was returning the wrong things from the 'end call function' when generating anonymous function shims. This was brought up here: #3576 (review) This should fix that problem.
| VD = VD->getCanonicalDecl(); | ||
| if (VD->isInAnonymousNamespace()) { | ||
| std::string TopShim = EmitSpecIdShims(OS, ShimCounter, VD); | ||
| OS << "namespace sycl {\n"; |
There was a problem hiding this comment.
Shouldn't we insert __SYCL_INLINE_NAMESPACE(cl) like it is done for integration header on line 4330?
@erichkeane
There was a problem hiding this comment.
I don't know, that wasn't a part of the spec I was given.
In order to get the specialization_id metadata to be usable in the
integration footer, we need to insert a 'shim' function at each
anonymous namespace so that we can do a lookup at the 'right' location,
then look it up again later by the type.
This implements all that functionality based on my latest understanding
of the design.