[PATCH] D68578: [HIP] Fix device stub name

This revision was automatically updated to reflect the committed changes. yaxunl marked 2 inline comments as done. Closed by commit rG22c457a869d5: [HIP] Fix device stub name (authored by yaxunl). Herald added a project: clang. Changed prior to commit: https://reviews.llvm.org/D68578?vs=248927&

[PATCH] D68578: [HIP] Fix device stub name

yaxunl marked 9 inline comments as done. yaxunl added inline comments. Comment at: clang/include/clang/AST/GlobalDecl.h:61 assert(!isa(D) && "Use other ctor with dtor decls!"); +assert(!D->hasAttr() && "Use other ctor with HIP kernels!"); tra wrote: >

[PATCH] D68578: [HIP] Fix device stub name

tra accepted this revision. tra added a comment. This revision is now accepted and ready to land. Few nits. LGTM otherwise. Comment at: clang/include/clang/AST/GlobalDecl.h:61 assert(!isa(D) && "Use other ctor with dtor decls!"); +assert(!D->hasAttr() && "Use other cto

[PATCH] D68578: [HIP] Fix device stub name

yaxunl updated this revision to Diff 248927. yaxunl added a comment. update patch CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68578/new/ https://reviews.llvm.org/D68578 Files: clang/include/clang/AST/GlobalDecl.h clang/lib/AST/Expr.cpp clang/lib/AST/ItaniumMangle.cpp clang/li

[PATCH] D68578: [HIP] Fix device stub name

yaxunl updated this revision to Diff 248544. yaxunl marked 11 inline comments as done. yaxunl added a comment. Revised by John's and Artem's comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68578/new/ https://reviews.llvm.org/D68578 Files: clang/include/clang/AST/GlobalDecl.h

[PATCH] D68578: [HIP] Fix device stub name

yaxunl marked 38 inline comments as done. yaxunl added inline comments. Comment at: clang/include/clang/AST/GlobalDecl.h:40 + Stub = 1, +}; + tra wrote: > rjmccall wrote: > > tra wrote: > > > rjmccall wrote: > > > > tra wrote: > > > > > rjmccall wrote: > > > > >

[PATCH] D68578: [HIP] Fix device stub name

tra added inline comments. Comment at: clang/include/clang/AST/GlobalDecl.h:40 + Stub = 1, +}; + rjmccall wrote: > tra wrote: > > rjmccall wrote: > > > tra wrote: > > > > rjmccall wrote: > > > > > The attribute here is `CUDAGlobalAttr`; should this be named in t

[PATCH] D68578: [HIP] Fix device stub name

rjmccall added inline comments. Comment at: clang/include/clang/AST/GlobalDecl.h:40 + Stub = 1, +}; + tra wrote: > rjmccall wrote: > > tra wrote: > > > rjmccall wrote: > > > > The attribute here is `CUDAGlobalAttr`; should this be named in terms > > > > of CUDA

[PATCH] D68578: [HIP] Fix device stub name

tra added inline comments. Comment at: clang/include/clang/AST/GlobalDecl.h:40 + Stub = 1, +}; + rjmccall wrote: > tra wrote: > > rjmccall wrote: > > > The attribute here is `CUDAGlobalAttr`; should this be named in terms of > > > CUDA, or is the CUDA model suf

[PATCH] D68578: [HIP] Fix device stub name

rjmccall added inline comments. Comment at: clang/include/clang/AST/GlobalDecl.h:40 + Stub = 1, +}; + tra wrote: > rjmccall wrote: > > The attribute here is `CUDAGlobalAttr`; should this be named in terms of > > CUDA, or is the CUDA model sufficiently different

[PATCH] D68578: [HIP] Fix device stub name

tra added a comment. Nice! Thank you for making these changes. Comment at: clang/include/clang/AST/GlobalDecl.h:40 + Stub = 1, +}; + rjmccall wrote: > The attribute here is `CUDAGlobalAttr`; should this be named in terms of > CUDA, or is the CUDA model suffic

[PATCH] D68578: [HIP] Fix device stub name

rjmccall added a comment. Thanks, I think this approach is really improving the existing code. Comment at: clang/include/clang/AST/GlobalDecl.h:40 + Stub = 1, +}; + The attribute here is `CUDAGlobalAttr`; should this be named in terms of CUDA, or is the CUDA

[PATCH] D68578: [HIP] Fix device stub name

yaxunl updated this revision to Diff 244988. yaxunl added a comment. Herald added a subscriber: kerbowa. Revised by John's comments. Introduced HIPKernelType for GlobalDecl so that we can use GlobalDecl to represent stub and kernel during host compilation. Revised mangler so that GlobalDecl carr

[PATCH] D68578: [HIP] Fix device stub name

rjmccall added a comment. I mean, I made a recommendation and you dismissed it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68578/new/ https://reviews.llvm.org/D68578 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.l

[PATCH] D68578: [HIP] Fix device stub name

yaxunl added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68578/new/ https://reviews.llvm.org/D68578 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68578: [HIP] Fix device stub name

yaxunl updated this revision to Diff 231966. yaxunl added a comment. Herald added subscribers: nhaehnle, jvesely. clean up and fix assertions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68578/new/ https://reviews.llvm.org/D68578 Files: clang/include/clang/Basic/Specifiers.h clan

[PATCH] D68578: [HIP] Fix device stub name

yaxunl updated this revision to Diff 231929. yaxunl added a comment. use calling convention to mangle the stub differently. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68578/new/ https://reviews.llvm.org/D68578 Files: clang/include/clang/Basic/Specifiers.h clang/lib/AST/ItaniumMa

[PATCH] D68578: [HIP] Fix device stub name

tra added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1099-1108 +auto *FD = const_cast((ND)); +if (auto *TD = cast(FD)->getPrimaryTemplate()) + FD = TD->getTemplatedDecl(); +auto OldDeclName = FD->getDeclName(); +auto NewNameStr = std::st

[PATCH] D68578: [HIP] Fix device stub name

yaxunl added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1099-1108 +auto *FD = const_cast((ND)); +if (auto *TD = cast(FD)->getPrimaryTemplate()) + FD = TD->getTemplatedDecl(); +auto OldDeclName = FD->getDeclName(); +auto NewNameStr = std:

[PATCH] D68578: [HIP] Fix device stub name

tra added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1099-1108 +auto *FD = const_cast((ND)); +if (auto *TD = cast(FD)->getPrimaryTemplate()) + FD = TD->getTemplatedDecl(); +auto OldDeclName = FD->getDeclName(); +auto NewNameStr = std::st

[PATCH] D68578: [HIP] Fix device stub name

yaxunl marked an inline comment as done. yaxunl added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1099-1108 +auto *FD = const_cast((ND)); +if (auto *TD = cast(FD)->getPrimaryTemplate()) + FD = TD->getTemplatedDecl(); +auto OldDeclName = FD->g

[PATCH] D68578: [HIP] Fix device stub name

tra added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1099-1108 +auto *FD = const_cast((ND)); +if (auto *TD = cast(FD)->getPrimaryTemplate()) + FD = TD->getTemplatedDecl(); +auto OldDeclName = FD->getDeclName(); +auto NewNameStr = std::st

[PATCH] D68578: [HIP] Fix device stub name

yaxunl updated this revision to Diff 229137. yaxunl added a comment. Attempt to prefix the kernel stub name on the fly. If we do not want to create two Decl's during parsing, and do not want to change the mangler, it seems the least invasive way to get the prefixed kernel name is to change it o

[PATCH] D68578: [HIP] Fix device stub name

rjmccall added a comment. There are a number of places in IRGen that pass around `GlobalDecl`s with the expectation that that's sufficient to uniquely identify a symbol. The fact that IRGen breaks down the GD at the last second before passing it to the mangler, rather than passing it to the ma

[PATCH] D68578: [HIP] Fix device stub name

yaxunl added a comment. In D68578#1737419 , @rjmccall wrote: > In D68578#1737415 , @yaxunl wrote: > > > In D68578#1737351 , @rjmccall > > wrote: > > > > > Distinguishing bet

[PATCH] D68578: [HIP] Fix device stub name

rjmccall added a comment. In D68578#1737415 , @yaxunl wrote: > In D68578#1737351 , @rjmccall wrote: > > > Distinguishing between multiple symbols associated with the same > > source-level declaration is the purpose

[PATCH] D68578: [HIP] Fix device stub name

yaxunl added a comment. In D68578#1737351 , @rjmccall wrote: > Distinguishing between multiple symbols associated with the same source-level > declaration is the purpose of the GlobalDecl abstraction. It seems GlobalDecl is just a wrapper for concrete D

[PATCH] D68578: [HIP] Fix device stub name

rjmccall added a comment. Distinguishing between multiple symbols associated with the same source-level declaration is the purpose of the GlobalDecl abstraction. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68578/new/ https://reviews.llvm.org/D68578

[PATCH] D68578: [HIP] Fix device stub name

tra added a comment. Apologies for the delay with my response. In D68578#1700819 , @t-tye wrote: > In D68578#1700652 , @tra wrote: > > > In D68578#1698864 , @t-tye wrote: >

[PATCH] D68578: [HIP] Fix device stub name

yaxunl added a comment. In D68578#1700652 , @tra wrote: > This patch proposes changing the source-level name for the stub. > Unfortunately the way it attempt to implement it is by doing the renaming > during mangling phase itself. This appears to be the

[PATCH] D68578: [HIP] Fix device stub name

t-tye added a comment. In D68578#1700652 , @tra wrote: > In D68578#1698864 , @t-tye wrote: > > > From a source language point of view, the device function comprises the > > code that is launched as a grid. We need

[PATCH] D68578: [HIP] Fix device stub name

tra added a comment. In D68578#1698864 , @t-tye wrote: > From a source language point of view, the device function comprises the code > that is launched as a grid. We need this fact to be present in the symbols > used. Only the device function should hav

[PATCH] D68578: [HIP] Fix device stub name

yaxunl added a comment. In D68578#1698864 , @t-tye wrote: > I am a little unclear what this patch is doing as it is mentioned that the > mangled name has a _stub in it. My understanding is that the intention was to > create a distinct unmangled name for

[PATCH] D68578: [HIP] Fix device stub name

t-tye added a comment. In D68578#1697898 , @tra wrote: > In D68578#1697851 , @yaxunl wrote: > > > In D68578#1697822 , @tra wrote: > > > > > Could you elaborate on how exactly

[PATCH] D68578: [HIP] Fix device stub name

tra added a comment. In D68578#1697851 , @yaxunl wrote: > In D68578#1697822 , @tra wrote: > > > Could you elaborate on how exactly current implementation does not work? > > > > I would expect the kernel and the stub

[PATCH] D68578: [HIP] Fix device stub name

hliao added inline comments. Comment at: lib/CodeGen/CGCUDANV.cpp:235 CGF.CGM.getContext().getTargetInfo().getCXXABI())) || + CGF.getLangOpts().HIP || getDeviceStubName(getDeviceSideName(CGF.CurFuncDecl)) == keeping the original asse

[PATCH] D68578: [HIP] Fix device stub name

yaxunl added a comment. In D68578#1697822 , @tra wrote: > Could you elaborate on how exactly current implementation does not work? > > I would expect the kernel and the stub to be two distinct entities, as far as > debugger is concerned. It does have enou

[PATCH] D68578: [HIP] Fix device stub name

tra added a comment. Could you elaborate on how exactly current implementation does not work? I would expect the kernel and the stub to be two distinct entities, as far as debugger is concerned. It does have enough information to track each independently (different address, .stub suffix, perhap

[PATCH] D68578: [HIP] Fix device stub name

yaxunl created this revision. yaxunl added reviewers: tra, rjmccall, hliao. Herald added a subscriber: erik.pilkington. HIP emits a device stub function for each kernel in host code. The HIP debugger requires device stub function to have a different unmangled name as the kernel. Currently the n