Page MenuHomePhabricator

Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name
ClosedPublic

Authored by erichkeane on May 25 2021, 1:32 PM.

Details

Summary

The original version of this was reverted, and @rjmcall provided some
advice to architect a new solution. This is that solution.

This implements a builtin to provide a unique name that is stable across
compilations of this TU for the purposes of implementing the library
component of the unnamed kernel feature of SYCL. It does this by
running the Itanium mangler with a few modifications.

Because it is somewhat common to wrap non-kernel-related lambdas in
macros that aren't present on the device (such as for logging), this
uniquely generates an ID for all lambdas involved in the naming of a
kernel. It uses the lambda-mangling number to do this, except replaces
this with its own number (starting at 10000 for readabililty reasons)
for lambdas used to name a kernel.

Additionally, this implements itself as constexpr with a slight catch:
if a name would be invalidated by the use of this lambda in a later
kernel invocation, it is diagnosed as an error (see the Sema tests).

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
erichkeane requested review of this revision.May 25 2021, 1:32 PM

Apply the clang-format patch from CI, also apply the unused variable fix clang-tidy suggested. The rest of the issues in clang-tidy are identifying functions in a recursive call-chain, but they are all part of the various visitors, so I don't think I have a way to fix those.

rjmccall added inline comments.May 25 2021, 11:45 PM
clang/docs/LanguageExtensions.rst
2413

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.

clang/include/clang/AST/Expr.h
2045

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?

clang/include/clang/Serialization/ASTBitCodes.h
1965

UniQuestableName :)

Probably ought to use underscores here.

clang/lib/AST/ASTContext.cpp
11689

Can we assert that these methods are only called in SYCL? I'd hate to accidentally do this bookkeeping in other modes.

clang/lib/AST/ItaniumMangle.cpp
1964

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?

5040

The expectation is that these names match the spelling in the source, so this should include the __builtin prefix.

aaron.ballman added inline comments.May 26 2021, 4:48 AM
clang/include/clang/Basic/Attr.td
329 ↗(On Diff #347805)

Pretty sure you meant this. :-)

1184 ↗(On Diff #347805)

Is this change (and the above) intentionally part of this commit? I don't see a different use of sycl_kernel in the test cases, so it's a bit unclear.

clang/lib/AST/ASTContext.cpp
2461

Unintentional whitespace change?

clang/lib/AST/ItaniumMangle.cpp
5041–5045
clang/lib/AST/StmtPrinter.cpp
1085

I think this comment can be removed -- I think this is doing the correct thing, but adding a test case may help prove it.

clang/lib/CodeGen/CGExprScalar.cpp
1591

Do we need to give this a name with a reserved identifier to avoid linking conflicts?

clang/lib/Sema/TreeTransform.h
10195

Does this transformation need to happen in an unevaluated context?

erichkeane marked 6 inline comments as done.

Do all the things requested by @aaron.ballman and @rjmccall except for limiting the syntax to 'types only'. Currently evaluating whether this is something we can accept.

erichkeane retitled this revision from Reimplement __builtin_unique_stable_name- to Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name.May 26 2021, 9:21 AM
aaron.ballman added inline comments.May 26 2021, 10:41 AM
clang/docs/LanguageExtensions.rst
2408

Underlines look off now, so this likely causes Sphinx warnings.

2440–2443
clang/include/clang/AST/ASTContext.h
3183
clang/include/clang/AST/Expr.h
2042
clang/include/clang/Serialization/ASTBitCodes.h
1964
clang/lib/Basic/IdentifierTable.cpp
122

It looks like you formatted a bit more than you intended to.

clang/lib/Parse/ParseExpr.cpp
2329–2357
2339
erichkeane marked 7 inline comments as done.

Fix comments from aaron.

clang/docs/LanguageExtensions.rst
2413

We can live with that, that makes sense to me.

clang/lib/AST/ItaniumMangle.cpp
1964

I was unable to find a way to get the device lambda mangling number to work in this situation unfortunately, it seems to have significantly different needs from what we need here.

Part of what SYCL needs is the ability to 'recalculate' this number as we discover that a lambda is participating in naming a SYCL kernel. The DeviceLambdaMangling mechanism requires that it be evaluated as we are generating the lambdas. I couldn't find a mechanism to update them after the fact that wasn't messier than the callback mechanism.

As far as assuming that we are changing only the discriminator, that ends up being required since this is the only location where a lambda mangling is 'customizable', and we want it to remain demanglable.

clang/lib/Basic/IdentifierTable.cpp
122

Ugh... tis what I get for running clang-format on the whole patch :/

rjmccall added inline comments.May 26 2021, 11:40 AM
clang/docs/LanguageExtensions.rst
2443

These need to be updated for the rename. You should just grep the patch for the old name.

clang/include/clang/AST/Expr.h
2045

That said, I don't really have a strong objection to supporting either a type or an expression operand.

clang/lib/AST/ItaniumMangle.cpp
1964

Sorry, I didn't mean that you should try to make the SYCL logic just set a device mangling number; in fact, I meant almost the reverse. The device mangling number is ultimately a MangleContext-driven override of the discriminator choice, just like you're trying to add for SYCL. For SYCL, you're adding a generalized callback mechanism, which seems good. What I'm asking is that you go ahead and move the existing device-mangling logic in the mangler over to that callback mechanism, so that instead of setting an isDeviceMangleContext() bit on the MangleContext, that code will install an discriminator-override callback that returns the device lambda mangling number. Then we have one mechanism instead of two.

I think the right API for that callback is just to have it return an Optional<unsigned>, and then you use the normal discriminator if it returns None. And it should take an arbitrary Decl* so that it can override discriminators on non-lambda local declarations if it wants.

erichkeane added inline comments.May 26 2021, 11:45 AM
clang/docs/LanguageExtensions.rst
2443

Looks like there was 1 more in addition to this that Aaron hadn't found! It'll be fixed in the next version.

clang/include/clang/AST/Expr.h
2045

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.

clang/lib/AST/ItaniumMangle.cpp
1964

I think that should work... I'll look into it, thanks for the clarification!

Remove 'expression' form per suggestion. Still need to do the device-mangling-number removal/rewrite business.

Replace the DeviceLambdaManglingNumber mechanism with the callback mechanism.

Hopefully this is what you were thinking @rjmccall.

Woops! Last update was JUST the changes, and I forgot to squash. Here is the whole patch.

Thanks, that seems to work out cleanly.

clang/include/clang/AST/Expr.h
2045

Okay. So this doesn't really need trailing storage anymore, and the documentation needs to be updated to not mention the expression production.

clang/include/clang/AST/Mangle.h
174

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.

clang/lib/AST/ASTContext.cpp
11691

Could you add an isSYCL() method on LangOpts as a convenience for this?

clang/lib/Basic/IdentifierTable.cpp
159

I think people would generally happier if you parenthesized Flags & KEYSYCL, even though, yes, this does work under C precedence rules.

clang/lib/CodeGen/CGCUDANV.cpp
196

typo: additonal

erichkeane marked 4 inline comments as done.

Ok, this should get me up to date! Fixed a bunch of the comments referring to 'expression', added LangOpts::isSYCL, and changed it to DiscriminatorOverride.

Apply clang-format patch, except for the changes to IdentifierTable.

rjmccall accepted this revision.May 26 2021, 7:24 PM

LGTM, thanks!

This revision is now accepted and ready to land.May 26 2021, 7:24 PM
aaron.ballman added inline comments.May 27 2021, 4:09 AM
clang/include/clang/Basic/LangOptions.h
448

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.)

clang/test/CodeGenSYCL/unique_stable_name.cpp
87–90

@rjmccall -- any opinions or ideas on this? I think VLAs should likely behave the same as they do in sizeof, etc.

erichkeane added inline comments.May 27 2021, 6:06 AM
clang/include/clang/AST/Expr.h
2045

Oh! Good point! *doh!*

clang/include/clang/AST/Mangle.h
174

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.

clang/include/clang/Basic/LangOptions.h
448

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).

clang/test/CodeGenSYCL/unique_stable_name.cpp
87–90

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.

aaron.ballman added inline comments.May 27 2021, 6:20 AM
clang/include/clang/Basic/LangOptions.h
448

That's a fair point -- I'm happy with either approach. Using isSYCL() is similarly expressive but with less typing, so I'm fine with it.

clang/test/CodeGenSYCL/unique_stable_name.cpp
87–90

My feeling is: I think we should support the usual VLA semantics if SYCL supports VLAs in any way. If it's easy to do so as part of this patch then great, but if it continues to elude us, I'm totally fine doing it as a follow-up because this is definitely a wonky edge case.

erichkeane added inline comments.May 27 2021, 6:25 AM
clang/test/CodeGenSYCL/unique_stable_name.cpp
87–90

I looked into it at one point, and it seems that the VLA handling in sizeof (and similar builtins) is done quite manually in quite a few places, it seemed like a pretty involved amount of work. I'd still hope to do it as a part of a follow-up (keyed by a user mentioning they care).

So I checked up on this...
1- The SYCL "Language" spec doesn't support VLAs at all (the same way C++ doesnt :)).

2- In our to-be-upstreamed downstream, we enforce the SYCL language rule that VLAs aren't allowed to be passed to kernels as they aren't a sized type.

3- The offload 'spir' target that is used for SYCL ALSO disallows VLAs with the error: error: variable length arrays are not supported for the current target

So I think it would be quite a bit of twisting for someone to get this builtin to apply to a VLA. WDYT?

aaron.ballman added inline comments.May 27 2021, 6:28 AM
clang/test/CodeGenSYCL/unique_stable_name.cpp
87–90

So I think it would be quite a bit of twisting for someone to get this builtin to apply to a VLA. WDYT?

Sold on leaving this as a FIXME until someone finds an actual issue they care about. Thanks for checking on all this!

aaron.ballman accepted this revision.May 27 2021, 6:28 AM

LGTM, thanks Erich!

This revision was landed with ongoing or failed builds.May 27 2021, 7:12 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2021, 7:12 AM
martong removed a subscriber: martong.Wed, Jul 28, 9:22 AM