First attempt at __unique_stable_name #250

erichkeane · 2019-06-25T18:39:22Z

This implementation uses the keyword __unique_stable_name as an operator
that can take a type or expression, and will result in a constexpr
constant character array that (hopefully) uniquely represents the type
or type-of-expression being passed to it.

The unique representation is the normal mangled name of the type, except
in the cases of a lambda, where the ordinal "number" is just replaced
with LINE->COL. Macro expansions are also appended with '~' followed by
the LINE->COL that it appears in.

For example, a lambda declared in
'main' would look like _ZTSZ4mainEUlvE25->12 (line 25, column 12).

A Lambda that comes from a macro looks like:
_ZTSZ4mainEUlvE45->3~18->32 (macro invoked on 45/3, lambda defined
inside the macro on line 18, column 32).

Template instantiations based on the lambda would result in a name that
contains the lambda mangling, for example:
_ZTSZ3bazIZ4mainEUlvE25->12EvvEUlvE14->12

A function template named 'baz' that is instantiated with a lambda
declared in 'main' on line 25/col 12 has another macro in it, declared
on line 14, column 12.

Signed-off-by: Erich Keane [email protected]

This implementation uses the keyword __unique_stable_name as an operator that can take a type or expression, and will result in a constexpr constant character array that (hopefully) uniquely represents the type or type-of-expression being passed to it. The unique representation is the normal mangled name of the type, except in the cases of a lambda, where the ordinal "number" is just replaced with LINE->COL. Macro expansions are also appended with '~' followed by the LINE->COL that it appears in. For example, a lambda declared in 'main' would look like _ZTSZ4mainEUlvE25->12 (line 25, column 12). A Lambda that comes from a macro looks like: _ZTSZ4mainEUlvE45->3~18->32 (macro invoked on 45/3, lambda defined inside the macro on line 18, column 32). Template instantiations based on the lambda would result in a name that contains the lambda mangling, for example: _ZTSZ3bazIZ4mainEUlvE25->12EvvEUlvE14->12 A function template named 'baz' that is instantiated with a lambda declared in 'main' on line 25/col 12 has another macro in it, declared on line 14, column 12. Signed-off-by: Erich Keane <[email protected]>

bader · 2019-06-25T18:59:16Z

+@hfinkel.
We are planning to use this built-in to make naming kernels by user optional in case both device and host compilers support this built-in function.
You feedback is very welcome.

hfinkel · 2019-06-25T19:22:44Z

+@hfinkel.
We are planning to use this built-in to make naming kernels by user optional in case both device and host compilers support this built-in function.
You feedback is very welcome.

Thanks, @bader (and @erichkeane )! Making the kernel names optional will really help our adoption of SYCL.

Are you also working on Microsoft mangling support? As a quick note, your clang/test/CodeGenSYCL/unique-stable-name.cpp doesn't seem to test multiple nested macros.

erichkeane · 2019-06-25T19:31:58Z

+@hfinkel.
We are planning to use this built-in to make naming kernels by user optional in case both device and host compilers support this built-in function.
You feedback is very welcome.

Thanks, @bader (and @erichkeane )! Making the kernel names optional will really help our adoption of SYCL.

Are you also working on Microsoft mangling support? As a quick note, your clang/test/CodeGenSYCL/unique-stable-name.cpp doesn't seem to test multiple nested macros.

We don't require microsoft mangling, this is simply a "unique" and "stable" name that just so happens to be implemented in terms of the Itanium mangling structure.

I'll add that test when I ge ta shot.

Signed-off-by: Erich Keane <[email protected]>

erichkeane · 2019-06-25T20:35:30Z

@hfinkel Test Case Added.

bader · 2019-06-27T14:14:45Z

clang/lib/Sema/SemaTemplateInstantiate.cpp

+      return E;
+
+    return getSema().BuildUniqueStableName(E->getLocation(), SubExpr.get());
+  } else if (E->getIdentKind() == PredefinedExpr::UniqueStableNameType) {


No else after return.

Signed-off-by: Erich Keane <[email protected]>

keryell

Super useful!
I would love more comments.
Add some documentation so it can land to https://clang.llvm.org/docs/LanguageExtensions.html
It would be nice to work with ISO C++ SG7 to have this feature in the standard.

clang/include/clang/AST/Expr.h

keryell · 2019-06-28T15:58:13Z

clang/include/clang/Basic/TokenKinds.def

@@ -436,6 +436,9 @@ KEYWORD(L__FUNCSIG__                , KEYMS)
 TYPE_TRAIT_1(__is_interface_class, IsInterfaceClass, KEYMS)
 TYPE_TRAIT_1(__is_sealed, IsSealed, KEYMS)

+// Extensions for SYCL.


Elaborate the comment to say what it does.

This is a category heading, it isn't directly associated with the keyword itself. If you see the rest of the file, that is the format of comments in this file.

Probably, we can move this built-in to some "generic" clang built-ins category.

clang/lib/AST/Expr.cpp

clang/lib/AST/ItaniumMangle.cpp

clang/lib/Sema/SemaExpr.cpp

keryell · 2019-06-28T16:12:13Z

clang/lib/Sema/SemaExpr.cpp

@@ -3205,6 +3205,65 @@ ExprResult Sema::BuildPredefinedExpr(SourceLocation Loc,

  return PredefinedExpr::Create(Context, Loc, ResTy, IK, SL);
 }


+1 to Ronan's comment to add a new line here.

clang/lib/Sema/SemaExpr.cpp

Signed-off-by: Erich Keane <[email protected]>

erichkeane · 2019-06-28T16:38:22Z

Super useful!
I would love more comments.
Add some documentation so it can land to https://clang.llvm.org/docs/LanguageExtensions.html
It would be nice to work with ISO C++ SG7 to have this feature in the standard.

Patch incoming.

A feature like this fits very well in with the Reflection work, though I haven't been participating in SG7 very much lately. This is implemented more like a builtin that would be used as a building block for such a proposal.

As far as putting this in LangExtensions, Clang typically doesn't document builtins in lang-extensions. This is a building block for omitting the kernel name in SYCL functions, so I suspect this is best documented as a part of that.

The intent of this patch is to unblock that kernel naming effort, so I suspect this is a first-of-many patch for this builtin.

hfinkel · 2019-06-28T16:46:57Z

Super useful!
I would love more comments.
Add some documentation so it can land to https://clang.llvm.org/docs/LanguageExtensions.html
It would be nice to work with ISO C++ SG7 to have this feature in the standard.

Patch incoming.

A feature like this fits very well in with the Reflection work, though I haven't been participating in SG7 very much lately. This is implemented more like a builtin that would be used as a building block for such a proposal.

As far as putting this in LangExtensions, Clang typically doesn't document builtins in lang-extensions. This is a building block for omitting the kernel name in SYCL functions, so I suspect this is best documented as a part of that.

The intent of this patch is to unblock that kernel naming effort, so I suspect this is a first-of-many patch for this builtin.

@erichkeane , we do, see https://clang.llvm.org/docs/LanguageExtensions.html#builtin-functions - and we should document this as well.

Regarding a proposal for WG21, I'm happy to work with you and anyone else interested on a proposal. For those of you attending the next WG21 meeting, I'm also happy to chat about this in person. That seems useful.

erichkeane · 2019-06-28T16:52:11Z

Super useful!
I would love more comments.
Add some documentation so it can land to https://clang.llvm.org/docs/LanguageExtensions.html
It would be nice to work with ISO C++ SG7 to have this feature in the standard.

Patch incoming.
A feature like this fits very well in with the Reflection work, though I haven't been participating in SG7 very much lately. This is implemented more like a builtin that would be used as a building block for such a proposal.
As far as putting this in LangExtensions, Clang typically doesn't document builtins in lang-extensions. This is a building block for omitting the kernel name in SYCL functions, so I suspect this is best documented as a part of that.
The intent of this patch is to unblock that kernel naming effort, so I suspect this is a first-of-many patch for this builtin.

@erichkeane , we do, see https://clang.llvm.org/docs/LanguageExtensions.html#builtin-functions - and we should document this as well.

Regarding a proposal for WG21, I'm happy to work with you and anyone else interested on a proposal. For those of you attending the next WG21 meeting, I'm also happy to chat about this in person. That seems useful.

Huh, I've never documented one there before! I'll work on it then. As for the proposal, perhaps the 3 of us should discuss it in Cologne. I'd presume that we'd need to find a way to fit this into the Reflection TS/Reflection Constexpr proposals in a way that didn't look terrible.

Additionally, we'd likely wish to propose an actual standardized naming convention here. This patch chooses a modified Itanium ABI, but any string would work.

keryell · 2019-06-28T16:55:30Z

I will skip the Köln meeting unfortunately... :-( Belfast?

erichkeane · 2019-06-28T16:56:09Z

I will skip the Köln meeting unfortunately... :-( Belfast?

Cya there.

Signed-off-by: Erich Keane <[email protected]>

clang/include/clang/Basic/TokenKinds.def

erichkeane · 2019-06-30T13:07:42Z

Interestingly, I got the reverse comment on the initial patch (that it was only necessary in SYCL mode). On Sunday, June 30, 2019 rolandschulz <[email protected]> wrote: @rolandschulz commented on this pull request. In clang/include/clang/Basic/TokenKinds.def:

@@ -436,6 +436,9 @@ KEYWORD(L__FUNCSIG__ , KEYMS)

TYPE_TRAIT_1(__is_interface_class, IsInterfaceClass, KEYMS) TYPE_TRAIT_1(__is_sealed, IsSealed, KEYMS) +// Extensions for SYCL. +KEYWORD(__unique_stable_name , KEYSYCL) It is also needed in standard C++ mode for the host compilation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Signed-off-by: Erich Keane <[email protected]>

clang/docs/LanguageExtensions.rst

clang/include/clang/AST/Expr.h

bader · 2019-07-01T20:52:21Z

clang/include/clang/Basic/TokenKinds.def

@@ -436,6 +436,9 @@ KEYWORD(L__FUNCSIG__                , KEYMS)
 TYPE_TRAIT_1(__is_interface_class, IsInterfaceClass, KEYMS)
 TYPE_TRAIT_1(__is_sealed, IsSealed, KEYMS)

+// Extensions for SYCL.


Probably, we can move this built-in to some "generic" clang built-ins category.

bader · 2019-07-01T20:57:15Z

clang/lib/Sema/SemaExpr.cpp

@@ -3205,6 +3205,65 @@ ExprResult Sema::BuildPredefinedExpr(SourceLocation Loc,

  return PredefinedExpr::Create(Context, Loc, ResTy, IK, SL);
 }


+1 to Ronan's comment to add a new line here.

Signed-off-by: Erich Keane <[email protected]>

erichkeane · 2019-07-02T13:32:54Z

I'm not authorized to merge this pull request, so I don't know should be doing it.

bader · 2019-07-02T14:06:46Z

I'd like to wait a bit to see if other reviewers have additional comments to the latest version of the patch and if there are no objections, I'll merge this patch tomorrow.
@erichkeane, thanks for working on this!

CONFLICT (content): Merge conflict in clang/lib/CodeGen/CGExpr.cpp

erichkeane requested review from bader, rolandschulz and kbobrovs June 25, 2019 18:40

Add Macro containing a Macro test case

ddd09b0

Signed-off-by: Erich Keane <[email protected]>

erichkeane force-pushed the private/erichkeane/unique-stable-name branch from 4c30e4a to ddd09b0 Compare June 25, 2019 20:34

bader previously approved these changes Jun 27, 2019

View reviewed changes

Fix else-after-return as @bader requested.

e10cd3e

Signed-off-by: Erich Keane <[email protected]>

erichkeane dismissed bader’s stale review via e10cd3e June 27, 2019 15:06

keryell reviewed Jun 28, 2019

View reviewed changes

respond to @keryell's commentsx

3431b5e

Signed-off-by: Erich Keane <[email protected]>

Document __unique_stable_name in LanguageExtensions.rst

23414ed

Signed-off-by: Erich Keane <[email protected]>

bader self-assigned this Jun 28, 2019

rolandschulz reviewed Jun 30, 2019

View reviewed changes

clang/include/clang/Basic/TokenKinds.def Outdated Show resolved Hide resolved

Enable __unqiue_stable_name in all configs so the host can use it too

52641a2

Signed-off-by: Erich Keane <[email protected]>

bader requested changes Jul 1, 2019

View reviewed changes

Respond to @bader's review comments

668ed23

Signed-off-by: Erich Keane <[email protected]>

bader approved these changes Jul 1, 2019

View reviewed changes

bader merged commit 44d6030 into sycl Jul 5, 2019

illuhad mentioned this pull request Jul 15, 2019

Work around Clang name mangling bug by using custom kernel names AdaptiveCpp/AdaptiveCpp#77

Merged

bader deleted the private/erichkeane/unique-stable-name branch July 31, 2019 19:50

vladimirlaz pushed a commit to vladimirlaz/llvm that referenced this pull request Nov 21, 2019

Merge from 'master' to 'sycl-web' (intel#250)

438bedc

CONFLICT (content): Merge conflict in clang/lib/CodeGen/CGExpr.cpp

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First attempt at __unique_stable_name #250

First attempt at __unique_stable_name #250

erichkeane commented Jun 25, 2019

bader commented Jun 25, 2019

hfinkel commented Jun 25, 2019

erichkeane commented Jun 25, 2019

erichkeane commented Jun 25, 2019

bader Jun 27, 2019

keryell left a comment

keryell Jun 28, 2019

erichkeane Jun 28, 2019

bader Jul 1, 2019

keryell Jun 28, 2019

bader Jul 1, 2019 •

edited

Loading

erichkeane commented Jun 28, 2019

hfinkel commented Jun 28, 2019

erichkeane commented Jun 28, 2019

keryell commented Jun 28, 2019

erichkeane commented Jun 28, 2019

erichkeane commented Jun 30, 2019 via email

bader Jul 1, 2019

bader Jul 1, 2019 •

edited

Loading

erichkeane commented Jul 2, 2019

bader commented Jul 2, 2019

		@@ -3205,6 +3205,65 @@ ExprResult Sema::BuildPredefinedExpr(SourceLocation Loc,

		return PredefinedExpr::Create(Context, Loc, ResTy, IK, SL);
		}

First attempt at __unique_stable_name #250

First attempt at __unique_stable_name #250

Conversation

erichkeane commented Jun 25, 2019

bader commented Jun 25, 2019

hfinkel commented Jun 25, 2019

erichkeane commented Jun 25, 2019

erichkeane commented Jun 25, 2019

bader Jun 27, 2019

Choose a reason for hiding this comment

keryell left a comment

Choose a reason for hiding this comment

keryell Jun 28, 2019

Choose a reason for hiding this comment

erichkeane Jun 28, 2019

Choose a reason for hiding this comment

bader Jul 1, 2019

Choose a reason for hiding this comment

keryell Jun 28, 2019

Choose a reason for hiding this comment

bader Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

erichkeane commented Jun 28, 2019

hfinkel commented Jun 28, 2019

erichkeane commented Jun 28, 2019

keryell commented Jun 28, 2019

erichkeane commented Jun 28, 2019

erichkeane commented Jun 30, 2019 via email

bader Jul 1, 2019

Choose a reason for hiding this comment

bader Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

erichkeane commented Jul 2, 2019

bader commented Jul 2, 2019

bader Jul 1, 2019 •

edited

Loading

bader Jul 1, 2019 •

edited

Loading