[PATCH] D63335: [HIP] Change kernel stub name again

[Michael Liao via Phabricator via cfe-commits]

hliao updated this revision to Diff 204856.
hliao added a comment.

Just revise the interface for device kernel stubbing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335

Files:
  clang/lib/CodeGen/CGCUDANV.cpp
  clang/lib/CodeGen/CGCUDARuntime.h
  clang/lib/CodeGen/CodeGenModule.cpp


Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1088,13 +1088,10 @@
   const auto *ND = cast(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 
-  // Postfix kernel stub names with .stub to differentiate them from kernel
-  // names in device binaries. This is to facilitate the debugger to find
-  // the correct symbols for kernels in the device binary.
+  // Derive the kernel stub from CUDA runtime.
   if (auto *FD = dyn_cast(GD.getDecl()))
-if (getLangOpts().HIP && !getLangOpts().CUDAIsDevice &&
-FD->hasAttr())
-  MangledName = MangledName + ".stub";
+if (!getLangOpts().CUDAIsDevice && FD->hasAttr())
+  MangledName = getCUDARuntime().getDeviceStubName(MangledName);
 
   auto Result = Manglings.insert(std::make_pair(MangledName, GD));
   return MangledDeclNames[CanonicalGD] = Result.first->first();
Index: clang/lib/CodeGen/CGCUDARuntime.h
===
--- clang/lib/CodeGen/CGCUDARuntime.h
+++ clang/lib/CodeGen/CGCUDARuntime.h
@@ -15,6 +15,8 @@
 #ifndef LLVM_CLANG_LIB_CODEGEN_CGCUDARUNTIME_H
 #define LLVM_CLANG_LIB_CODEGEN_CGCUDARUNTIME_H
 
+#include "llvm/ADT/StringRef.h"
+
 namespace llvm {
 class Function;
 class GlobalVariable;
@@ -63,6 +65,9 @@
   /// Returns a module cleanup function or nullptr if it's not needed.
   /// Must be called after ModuleCtorFunction
   virtual llvm::Function *makeModuleDtorFunction() = 0;
+
+  /// Construct and return the stub name of a kernel.
+  virtual std::string getDeviceStubName(llvm::StringRef Name) const = 0;
 };
 
 /// Creates an instance of a CUDA runtime class.
Index: clang/lib/CodeGen/CGCUDANV.cpp
===
--- clang/lib/CodeGen/CGCUDANV.cpp
+++ clang/lib/CodeGen/CGCUDANV.cpp
@@ -132,6 +132,8 @@
   llvm::Function *makeModuleCtorFunction() override;
   /// Creates module destructor function
   llvm::Function *makeModuleDtorFunction() override;
+  /// Construct and return the stub name of a kernel.
+  std::string getDeviceStubName(llvm::StringRef Name) const override;
 };
 
 }
@@ -217,10 +219,20 @@
 
 void CGNVCUDARuntime::emitDeviceStub(CodeGenFunction &CGF,
  FunctionArgList &Args) {
-  assert(getDeviceSideName(CGF.CurFuncDecl) == CGF.CurFn->getName() ||
- getDeviceSideName(CGF.CurFuncDecl) + ".stub" == CGF.CurFn->getName() 
||
- CGF.CGM.getContext().getTargetInfo().getCXXABI() !=
- CGF.CGM.getContext().getAuxTargetInfo()->getCXXABI());
+  // Ensure either we have different ABIs between host and device compilations,
+  // says host compilation following MSVC ABI but device compilation follows
+  // Itanium C++ ABI or, if they follow the same ABI, kernel names after
+  // mangling should be same after name stubbing. The later checking is very
+  // important as the device kernel name being mangled in host-compilation is
+  // used to resolve the device binaries to be executed. Inconsistent naming
+  // result in undefined behavior. Even though we cannot check that naming
+  // directly between host- and device-compilations, the host- and
+  // device-mangling in host compilation could help catch certain ones.
+  assert((CGF.CGM.getContext().getAuxTargetInfo() &&
+  (CGF.CGM.getContext().getAuxTargetInfo()->getCXXABI() !=
+   CGF.CGM.getContext().getTargetInfo().getCXXABI())) ||
+ getDeviceStubName(getDeviceSideName(CGF.CurFuncDecl)) ==
+ CGF.CurFn->getName());
 
   EmittedKernels.push_back({CGF.CurFn, CGF.CurFuncDecl});
   if (CudaFeatureEnabled(CGM.getTarget().getSDKVersion(),
@@ -780,6 +792,12 @@
   return ModuleDtorFunc;
 }
 
+std::string CGNVCUDARuntime::getDeviceStubName(llvm::StringRef Name) const {
+  if (!CGM.getLangOpts().HIP)
+return Name;
+  return std::move((Name + ".stub").str());
+}
+
 CGCUDARuntime *CodeGen::CreateNVCUDARuntime(CodeGenModule &CGM) {
   return new CGNVCUDARuntime(CGM);
 }


Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1088,13 +1088,10 @@
   const auto *ND = cast(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 
-  // Postfix kernel stub names with .stub to differentiate them from kernel
-  // names in device binaries. This is to facilitate the debugger to find
-  // the cor

[PATCH] D63335: [HIP] Change kernel stub name again

[Michael Liao via Phabricator via cfe-commits]

hliao added a comment.

In D63335#1544320 , @tra wrote:

> In D63335#1544315 , @hliao wrote:
>
> > > Sorry, I still don't think I understand the reasons for this change. The 
> > > stub and the kernel do have a different name now. I don't quite get it 
> > > why the debugger can differentiate the names when they differ by prefix, 
> > > but can't when they differ by suffix. It sounds like an attempt to work 
> > > around a problem somewhere else.
> > > 
> > > Could you talk to the folks requesting the change and get more details on 
> > > what exactly we need to do here and, more importantly, why.
> >
> > But, after unmangling, debugger still could match both as they are almost 
> > identical excep the final variants, like `clone`. The debugger will set all 
> > locations matching that specified kernel name.
>
>
> OK, so the real issue is that demangled name looks identical to debugger.
>  One way to deal with that is to , essentially, break mangling in compiler.
>  Another would be to teach debugger how to distinguish the stub from the 
> kernel using additional information likely available to debugger (i.e. 
> mangled name or the location of the symbol -- is it in the host binary or in 
> the GPU binary).
>
> I would argue that breaking mangling is not the best choice here. 
>  I think debugger does have sufficient information to deal with this and that 
> would be the right place to deal with the issue.


em, I did push the later as well, :(. OK, I will simplify the patch to change 
any functionality but move the calculation of device name into a common 
interface. So that, vendor could adjust that internally with minimal change. OK?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Artem Belevich via Phabricator via cfe-commits]

tra requested changes to this revision.
tra added a comment.
This revision now requires changes to proceed.

In D63335#1544315 , @hliao wrote:

> > Sorry, I still don't think I understand the reasons for this change. The 
> > stub and the kernel do have a different name now. I don't quite get it why 
> > the debugger can differentiate the names when they differ by prefix, but 
> > can't when they differ by suffix. It sounds like an attempt to work around 
> > a problem somewhere else.
> > 
> > Could you talk to the folks requesting the change and get more details on 
> > what exactly we need to do here and, more importantly, why.
>
> But, after unmangling, debugger still could match both as they are almost 
> identical excep the final variants, like `clone`. The debugger will set all 
> locations matching that specified kernel name.


OK, so the real issue is that demangled name looks identical to debugger.
One way to deal with that is to , essentially, break mangling in compiler.
Another would be to teach debugger how to distinguish the stub from the kernel 
using additional information likely available to debugger (i.e. mangled name or 
the location of the symbol -- is it in the host binary or in the GPU binary).

I would argue that breaking mangling is not the best choice here. 
I think debugger does have sufficient information to deal with this and that 
would be the right place to deal with the issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D63335#1544026 , @hliao wrote:

> Is it OK for us to mangle `__device_stub __` as the nested name into the 
> original one, says, we prepend `_ZN15__device_stub__E`, so that we have 
> `_ZN15__device_stub__E10kernelfuncIiEvv`
>
> and
>
>   $ c++filt _ZN15__device_stub__E10kernelfuncIiEvv
>   __device_stub__(kernelfunc, void, void)
>


I don't think it's a good idea. While it demangles to something, it's not what 
the demangled name should be. Stub's signature should match that of the kernel.

In D63335#1544021 , @hliao wrote:

> Yeah, I understand that un-demangleable name causes lots of frustration. But, 
> based on what I learned, CUDA generated the similar thing, e.g. 
> `__device_stub__Z15transformKernelPfiif` is the stub function from cuda 10.1


NVCC does a lot of things differently. It does not mean it's a good reason for 
us to copy *all* of their choices.
Let's figure out the underlying reasons for this change and then we can figure 
out about what's the right thing to do here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Michael Liao via Phabricator via cfe-commits]

hliao added a comment.

In D63335#1544311 , @tra wrote:

> In D63335#1544019 , @hliao wrote:
>
> > In D63335#1543854 , @tra wrote:
> >
> > > In D63335#1543845 , @hliao wrote:
> > >
> > > > it's requested from debugger people. they don't want to the host-side 
> > > > stub could match the device-side kernel function name. the previous 
> > > > scheme cannot prevent that.
> > >
> > >
> > > I understand that you want a different name for the stub. My question is 
> > > why the ".stub" suffix was not sufficient and how does having a prefix 
> > > instead helps? Making the name un-demangleable is undesirable, IMO. There 
> > > should be a good reason to justify it.
> >
> >
> > it's based on debugger people told me, with ".stub", the debugger still 
> > could find it match the original device kernel even though it could find 
> > both of them. But, they want to match the original one only and leave the 
> > stub one intentionally unmatched.
>
>
> Sorry, I still don't think I understand the reasons for this change. The stub 
> and the kernel do have a different name now. I don't quite get it why the 
> debugger can differentiate the names when they differ by prefix, but can't 
> when they differ by suffix. It sounds like an attempt to work around a 
> problem somewhere else.
>
> Could you talk to the folks requesting the change and get more details on 
> what exactly we need to do here and, more importantly, why.


But, after unmangling, debugger still could match both as they are almost 
identical excep the final variants, like `clone`. The debugger will set all 
locations matching that specified kernel name.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D63335#1544019 , @hliao wrote:

> In D63335#1543854 , @tra wrote:
>
> > In D63335#1543845 , @hliao wrote:
> >
> > > it's requested from debugger people. they don't want to the host-side 
> > > stub could match the device-side kernel function name. the previous 
> > > scheme cannot prevent that.
> >
> >
> > I understand that you want a different name for the stub. My question is 
> > why the ".stub" suffix was not sufficient and how does having a prefix 
> > instead helps? Making the name un-demangleable is undesirable, IMO. There 
> > should be a good reason to justify it.
>
>
> it's based on debugger people told me, with ".stub", the debugger still could 
> find it match the original device kernel even though it could find both of 
> them. But, they want to match the original one only and leave the stub one 
> intentionally unmatched.


Sorry, I still don't think I understand the reasons for this change. The stub 
and the kernel do have a different name now. I don't quite get it why the 
debugger can differentiate the names when they differ by prefix, but can't when 
they differ by suffix. It sounds like an attempt to work around a problem 
somewhere else.

Could you talk to the folks requesting the change and get more details on what 
exactly we need to do here and, more importantly, why.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Michael Liao via Phabricator via cfe-commits]

hliao added a comment.

In D63335#1543854 , @tra wrote:

> In D63335#1543845 , @hliao wrote:
>
> > it's requested from debugger people. they don't want to the host-side stub 
> > could match the device-side kernel function name. the previous scheme 
> > cannot prevent that.
>
>
> I understand that you want a different name for the stub. My question is why 
> the ".stub" suffix was not sufficient and how does having a prefix instead 
> helps? Making the name un-demangleable is undesirable, IMO. There should be a 
> good reason to justify it.


Is it OK for us to mangle `__device_stub __` as the nested name into the 
original one, says, we prepend `_ZN15__device_stub__E`, so that we have 
`_ZN15__device_stub__E10kernelfuncIiEvv`

and

$ c++filt _ZN15__device_stub__E10kernelfuncIiEvv
__device_stub__(kernelfunc, void, void)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Michael Liao via Phabricator via cfe-commits]

hliao added a comment.

In D63335#1543854 , @tra wrote:

> In D63335#1543845 , @hliao wrote:
>
> > it's requested from debugger people. they don't want to the host-side stub 
> > could match the device-side kernel function name. the previous scheme 
> > cannot prevent that.
>
>
> I understand that you want a different name for the stub. My question is why 
> the ".stub" suffix was not sufficient and how does having a prefix instead 
> helps? Making the name un-demangleable is undesirable, IMO. There should be a 
> good reason to justify it.


Yeah, I understand that un-demangleable name causes lots of frustration. But, 
based on what I learned, CUDA generated the similar thing, e.g. 
`__device_stub__Z15transformKernelPfiif` is the stub function from cuda 10.1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Michael Liao via Phabricator via cfe-commits]

hliao added a comment.

In D63335#1543854 , @tra wrote:

> In D63335#1543845 , @hliao wrote:
>
> > it's requested from debugger people. they don't want to the host-side stub 
> > could match the device-side kernel function name. the previous scheme 
> > cannot prevent that.
>
>
> I understand that you want a different name for the stub. My question is why 
> the ".stub" suffix was not sufficient and how does having a prefix instead 
> helps? Making the name un-demangleable is undesirable, IMO. There should be a 
> good reason to justify it.


it's based on debugger people told me, with ".stub", the debugger still could 
find it match the original device kernel even though it could find both of 
them. But, they want to match the original one only and leave the stub one 
intentionally unmatched.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Michael Liao via Phabricator via cfe-commits]

hliao marked an inline comment as done.
hliao added inline comments.



Comment at: clang/lib/CodeGen/CGCUDANV.cpp:789
+return Name;
+  return std::move(("__device_stub__" + Name).str());
+}

tra wrote:
> I suspect `return "__device_stub__" + Name;` would do. StringRef will convert 
> to std::string and copy elision should avoid unnecessary copy.
"__device__stub__" + Name results in Twine, where not copy is generated. Only 
the final str() converts Twine into std::string involving copies. Otherwise, 
there's one copy from Name to std::string and another copy by std::string 
operator+, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D63335#1543845 , @hliao wrote:

> it's requested from debugger people. they don't want to the host-side stub 
> could match the device-side kernel function name. the previous scheme cannot 
> prevent that.


I understand that you want a different name for the stub. My question is why 
the ".stub" suffix was not sufficient and how does having a prefix instead 
helps? Making the name un-demangleable is undesirable, IMO. There should be a 
good reason to justify it.




Comment at: clang/lib/CodeGen/CGCUDANV.cpp:222-226
+  assert((CGF.CGM.getContext().getAuxTargetInfo() &&
+  (CGF.CGM.getContext().getAuxTargetInfo()->getCXXABI() !=
+   CGF.CGM.getContext().getTargetInfo().getCXXABI())) ||
+ getDeviceStubName(getDeviceSideName(CGF.CurFuncDecl)) ==
+ CGF.CurFn->getName());

hliao wrote:
> tra wrote:
> > I'm not sure I understand what exactly this assertion checks.
> > The condition appears to be true is host/device ABIs are different OR the 
> > name of the current function is the same as the (possibly mangled) 
> > device-side name + __device_stub_ prefix.
> > 
> > While the first part makes sense, I'm not sure I understand the name 
> > comparison part.
> > Could you tell me more and, maybe, add a comment explaining what's going on 
> > here.
> The second is to ensure, if, under the same ABI, kernel stub name derived 
> from device-side name mangling should be the same the sub name generated from 
> host-side, CGF.CurFn->getName() is the mangled named from host compilation
This definitely needs a comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Michael Liao via Phabricator via cfe-commits]

hliao marked an inline comment as done.
hliao added inline comments.



Comment at: clang/lib/CodeGen/CGCUDANV.cpp:222-226
+  assert((CGF.CGM.getContext().getAuxTargetInfo() &&
+  (CGF.CGM.getContext().getAuxTargetInfo()->getCXXABI() !=
+   CGF.CGM.getContext().getTargetInfo().getCXXABI())) ||
+ getDeviceStubName(getDeviceSideName(CGF.CurFuncDecl)) ==
+ CGF.CurFn->getName());

hliao wrote:
> tra wrote:
> > I'm not sure I understand what exactly this assertion checks.
> > The condition appears to be true is host/device ABIs are different OR the 
> > name of the current function is the same as the (possibly mangled) 
> > device-side name + __device_stub_ prefix.
> > 
> > While the first part makes sense, I'm not sure I understand the name 
> > comparison part.
> > Could you tell me more and, maybe, add a comment explaining what's going on 
> > here.
> The second is to ensure, if, under the same ABI, kernel stub name derived 
> from device-side name mangling should be the same the sub name generated from 
> host-side, CGF.CurFn->getName() is the mangled named from host compilation
previous assertion expression gets the same goal, if ABI is different, the stub 
name from device-side should match the stub name from the host-side 
compilation. As we add a dedicated interface to the derive stub name, we could 
simplify the comparison to a single one.
Also, we put the simple condition checking ahead (a common practice) to reduce 
the overhead of string comparison


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Michael Liao via Phabricator via cfe-commits]

hliao marked an inline comment as done.
hliao added a comment.

it's requested from debugger people. they don't want to the host-side stub 
could match the device-side kernel function name. the previous scheme cannot 
prevent that.




Comment at: clang/lib/CodeGen/CGCUDANV.cpp:222-226
+  assert((CGF.CGM.getContext().getAuxTargetInfo() &&
+  (CGF.CGM.getContext().getAuxTargetInfo()->getCXXABI() !=
+   CGF.CGM.getContext().getTargetInfo().getCXXABI())) ||
+ getDeviceStubName(getDeviceSideName(CGF.CurFuncDecl)) ==
+ CGF.CurFn->getName());

tra wrote:
> I'm not sure I understand what exactly this assertion checks.
> The condition appears to be true is host/device ABIs are different OR the 
> name of the current function is the same as the (possibly mangled) 
> device-side name + __device_stub_ prefix.
> 
> While the first part makes sense, I'm not sure I understand the name 
> comparison part.
> Could you tell me more and, maybe, add a comment explaining what's going on 
> here.
The second is to ensure, if, under the same ABI, kernel stub name derived from 
device-side name mangling should be the same the sub name generated from 
host-side, CGF.CurFn->getName() is the mangled named from host compilation


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

Is there particular reason you need to switch to this naming scheme?

One issue with this patch is that demanglers will no longer be able to deal 
with the name. While they do know to ignore .stub suffix, they can't deal with 
`__device_stub_` prefix.
E.g:

  % c++filt __device_stub___Z10kernelfuncIiEvv
  __device_stub___Z10kernelfuncIiEvv
  % c++filt _Z10kernelfuncIiEvv.stub
  void kernelfunc() [clone .stub]






Comment at: clang/lib/CodeGen/CGCUDANV.cpp:222-226
+  assert((CGF.CGM.getContext().getAuxTargetInfo() &&
+  (CGF.CGM.getContext().getAuxTargetInfo()->getCXXABI() !=
+   CGF.CGM.getContext().getTargetInfo().getCXXABI())) ||
+ getDeviceStubName(getDeviceSideName(CGF.CurFuncDecl)) ==
+ CGF.CurFn->getName());

I'm not sure I understand what exactly this assertion checks.
The condition appears to be true is host/device ABIs are different OR the name 
of the current function is the same as the (possibly mangled) device-side name 
+ __device_stub_ prefix.

While the first part makes sense, I'm not sure I understand the name comparison 
part.
Could you tell me more and, maybe, add a comment explaining what's going on 
here.



Comment at: clang/lib/CodeGen/CGCUDANV.cpp:789
+return Name;
+  return std::move(("__device_stub__" + Name).str());
+}

I suspect `return "__device_stub__" + Name;` would do. StringRef will convert 
to std::string and copy elision should avoid unnecessary copy.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63335/new/

https://reviews.llvm.org/D63335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63335: [HIP] Change kernel stub name again

[Michael Liao via Phabricator via cfe-commits]

hliao created this revision.
hliao added reviewers: yaxunl, tra.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

- Prefix kernel stub with `__device_stub__` to avoid potential symbol name 
conflicts in debugger.
- Revise the interface to derive the stub name and simplify the assertion of it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D63335

Files:
  clang/lib/CodeGen/CGCUDANV.cpp
  clang/lib/CodeGen/CGCUDARuntime.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGenCUDA/kernel-stub-name.cu


Index: clang/test/CodeGenCUDA/kernel-stub-name.cu
===
--- clang/test/CodeGenCUDA/kernel-stub-name.cu
+++ clang/test/CodeGenCUDA/kernel-stub-name.cu
@@ -10,7 +10,7 @@
 __global__ void kernelfunc() {}
 
 // CHECK-LABEL: define{{.*}}@_Z8hostfuncv()
-// CHECK: call void @[[STUB:_Z10kernelfuncIiEvv.stub]]()
+// CHECK: call void @[[STUB:__device_stub___Z10kernelfuncIiEvv]]()
 void hostfunc(void) { kernelfunc<<<1, 1>>>(); }
 
 // CHECK: define{{.*}}@[[STUB]]
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1088,13 +1088,10 @@
   const auto *ND = cast(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 
-  // Postfix kernel stub names with .stub to differentiate them from kernel
-  // names in device binaries. This is to facilitate the debugger to find
-  // the correct symbols for kernels in the device binary.
+  // Derive the kernel stub from CUDA runtime.
   if (auto *FD = dyn_cast(GD.getDecl()))
-if (getLangOpts().HIP && !getLangOpts().CUDAIsDevice &&
-FD->hasAttr())
-  MangledName = MangledName + ".stub";
+if (!getLangOpts().CUDAIsDevice && FD->hasAttr())
+  MangledName = getCUDARuntime().getDeviceStubName(MangledName);
 
   auto Result = Manglings.insert(std::make_pair(MangledName, GD));
   return MangledDeclNames[CanonicalGD] = Result.first->first();
Index: clang/lib/CodeGen/CGCUDARuntime.h
===
--- clang/lib/CodeGen/CGCUDARuntime.h
+++ clang/lib/CodeGen/CGCUDARuntime.h
@@ -15,6 +15,8 @@
 #ifndef LLVM_CLANG_LIB_CODEGEN_CGCUDARUNTIME_H
 #define LLVM_CLANG_LIB_CODEGEN_CGCUDARUNTIME_H
 
+#include "llvm/ADT/StringRef.h"
+
 namespace llvm {
 class Function;
 class GlobalVariable;
@@ -63,6 +65,9 @@
   /// Returns a module cleanup function or nullptr if it's not needed.
   /// Must be called after ModuleCtorFunction
   virtual llvm::Function *makeModuleDtorFunction() = 0;
+
+  /// Construct and return the stub name of a kernel.
+  virtual std::string getDeviceStubName(llvm::StringRef Name) const = 0;
 };
 
 /// Creates an instance of a CUDA runtime class.
Index: clang/lib/CodeGen/CGCUDANV.cpp
===
--- clang/lib/CodeGen/CGCUDANV.cpp
+++ clang/lib/CodeGen/CGCUDANV.cpp
@@ -132,6 +132,8 @@
   llvm::Function *makeModuleCtorFunction() override;
   /// Creates module destructor function
   llvm::Function *makeModuleDtorFunction() override;
+  /// Construct and return the stub name of a kernel.
+  std::string getDeviceStubName(llvm::StringRef Name) const override;
 };
 
 }
@@ -217,10 +219,11 @@
 
 void CGNVCUDARuntime::emitDeviceStub(CodeGenFunction &CGF,
  FunctionArgList &Args) {
-  assert(getDeviceSideName(CGF.CurFuncDecl) == CGF.CurFn->getName() ||
- getDeviceSideName(CGF.CurFuncDecl) + ".stub" == CGF.CurFn->getName() 
||
- CGF.CGM.getContext().getTargetInfo().getCXXABI() !=
- CGF.CGM.getContext().getAuxTargetInfo()->getCXXABI());
+  assert((CGF.CGM.getContext().getAuxTargetInfo() &&
+  (CGF.CGM.getContext().getAuxTargetInfo()->getCXXABI() !=
+   CGF.CGM.getContext().getTargetInfo().getCXXABI())) ||
+ getDeviceStubName(getDeviceSideName(CGF.CurFuncDecl)) ==
+ CGF.CurFn->getName());
 
   EmittedKernels.push_back({CGF.CurFn, CGF.CurFuncDecl});
   if (CudaFeatureEnabled(CGM.getTarget().getSDKVersion(),
@@ -780,6 +783,12 @@
   return ModuleDtorFunc;
 }
 
+std::string CGNVCUDARuntime::getDeviceStubName(llvm::StringRef Name) const {
+  if (!CGM.getLangOpts().HIP)
+return Name;
+  return std::move(("__device_stub__" + Name).str());
+}
+
 CGCUDARuntime *CodeGen::CreateNVCUDARuntime(CodeGenModule &CGM) {
   return new CGNVCUDARuntime(CGM);
 }


Index: clang/test/CodeGenCUDA/kernel-stub-name.cu
===
--- clang/test/CodeGenCUDA/kernel-stub-name.cu
+++ clang/test/CodeGenCUDA/kernel-stub-name.cu
@@ -10,7 +10,7 @@
 __global__ void kernelfunc() {}
 
 // CHECK-LABEL: define{{.*}}@_Z8hostfuncv()
-// CHECK: call void @[[STUB:_Z10kernelfuncIiEvv.stub]]()
+// CHECK: call void @[[STUB:__device_stub___Z10kernelfuncIiEvv]]()
 void hostfunc(void) { kernelfu