[PATCH] D63335: [HIP] Add the interface deriving the stub name of device kernels.

[Michael Liao via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL363553: [HIP] Add the interface deriving the stub name of 
device kernels. (authored by hliao, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63335?vs=204856=205051#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63335

Files:
  cfe/trunk/lib/CodeGen/CGCUDANV.cpp
  cfe/trunk/lib/CodeGen/CGCUDARuntime.h
  cfe/trunk/lib/CodeGen/CodeGenModule.cpp


Index: cfe/trunk/lib/CodeGen/CGCUDARuntime.h
===
--- cfe/trunk/lib/CodeGen/CGCUDARuntime.h
+++ cfe/trunk/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: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp
@@ -1088,13 +1088,11 @@
   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.
+  // Adjust kernel stub mangling as we may need to be able to differentiate
+  // them from the kernel itself (e.g., for HIP).
   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: cfe/trunk/lib/CodeGen/CGCUDANV.cpp
===
--- cfe/trunk/lib/CodeGen/CGCUDANV.cpp
+++ cfe/trunk/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 ,
  FunctionArgList ) {
-  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 the 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 catching 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 ) {
   return new CGNVCUDARuntime(CGM);
 }


Index: cfe/trunk/lib/CodeGen/CGCUDARuntime.h
===
--- 

[PATCH] D63335: [HIP] Add the interface deriving the stub name of device kernels.

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl accepted this revision.
yaxunl added a comment.

LGTM. Thanks.


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] Add the interface deriving the stub name of device kernels.

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.

LGTM. This is a cleaner way to provide stub name tweaks.




Comment at: clang/lib/CodeGen/CGCUDANV.cpp:223
+  // 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

says  -> (e.g. )



Comment at: clang/lib/CodeGen/CGCUDANV.cpp:225
+  // 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

... should be `the same` ...
or, perhaps, `identical`



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1091
 
-  // 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()))

Adjust kernel stub mangling as we may need to be able to differentiate them 
from the kernel itself (e.g. for HIP).


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] Add the interface deriving the stub name of device kernels.

[Michael Liao via Phabricator via cfe-commits]

hliao added a comment.

In D63335#1544428 , @tra wrote:

> In D63335#1544324 , @hliao wrote:
>
> > > 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?
>
>
> :-( Sorry about that. I realize how frustrating that can be.
>
> Perhaps it's worth trying once more. You can argue that this change will have 
> trouble being upstreamed without a good technical explanation why it must be 
> done in the compiler. Perhaps they do have compelling reasons why it's hard 
> to do in the debugger, but without specific details from their end it appears 
> indistinguishable from a (possibly misguided) quick fix. It may help if you 
> could get the debugger folks to chime in directly on the review.


shall we review code refactoring first, so that that change could be just a 
single line change. Yes, I could post that later and drag in necessary stake 
holders.


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] Add the interface deriving the stub name of device kernels.

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D63335#1544324 , @hliao wrote:

> > 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?


:-( Sorry about that. I realize how frustrating that can be.

Perhaps it's worth trying once more. You can argue that this change will have 
trouble being upstreamed without a good technical explanation why it must be 
done in the compiler. Perhaps they do have compelling reasons why it's hard to 
do in the debugger, but without specific details from their end it appears 
indistinguishable from a (possibly misguided) quick fix. It may help if you 
could get the debugger folks to chime in directly on the review.


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