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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1099-1108
+    auto *FD = const_cast<NamedDecl *>((ND));
+    if (auto *TD = cast<FunctionDecl>(FD)->getPrimaryTemplate())
+      FD = TD->getTemplatedDecl();
+    auto OldDeclName = FD->getDeclName();
+    auto NewNameStr = std::string("__device_stub__") + OldDeclName.getAsString();
+    auto *NewId = &Context.Idents.get(NewNameStr);
+    auto NewDeclName = DeclarationName(NewId);
----------------
yaxunl wrote:
> tra wrote:
> > On one hand I like this patch variant much better than the one that changed the mangling itself.
> > On the other hand this code appears to reply on implementation details. I.e. we're setting new name on `FD` which may or may not be the same as `ND`, but we're always passing `ND` to `getMangledNameImpl()`. 
> > 
> > Perhaps we could implement name-tweaking as another `MultiVersionKind` which we already plumb into getMangledNameImpl() and which allows changing the name for target attributes & features.
> > 
> > 
> The mangled name of an instantiated template function does not depends on its own name, but on the template. If we do not want to depend on this implementation detail, it seems I have to clone the template and instantiate from the clone.
> 
> MultiVersion does not help us here since it only appends .postfix to mangled name. The obstacle we are facing is how to change the unmangled name.
> The mangled name of an instantiated template function does not depends on its own name, but on the template. If we do not want to depend on this implementation detail, it seems I have to clone the template and instantiate from the clone.

That would be putting more effort into working around the fact that `getMangledNameImpl()` doesa not provide a good API to change the name the way you need to. *That's* what needs to be addressed, IMO.

>MultiVersion does not help us here since it only appends .postfix to mangled name. The obstacle we are facing is how to change the unmangled name.

*Some* existing implementations append to the mangled name, but we can do other manipulations there, too. The string with the mangled name originates in `getMangledNameImpl` and we could do more than just append to it. We do not have to use the `MultiVersion` for that, either. E.g. we prepend `__regcall3__` to the names of functions with `CC_X86RegCall` calling convention. We could do something similar for the kernel stub, I wonder if we could just generate a unique name and be done with that?

Hmm. Unique name probably would not do if, let's say, a kernel is defined in one TU, but we need to call it from another TU. So, whichever way we change the name of the stub, it will need to be the same everywhere. 
You may want to add a test verifying that launching of declaration-only kernels uses the right name.

Consistency of name mangling means that we do need to include regular C++-mangled information. Which means we need to do the name tweaking deeper down. How about using calling conventions? It's been suggested in the past that a lot of shenanigans around kernel launches could/should be done as a different calling convention. One of the things affected by the calling convention is mangling and we can add prefix there: https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/Mangle.cpp#L164

We could tag host-side kernel with 'kernel call' calling convention on the host side and then plumb prefixing to be done similar to `__regcall3__`.

If that works that may be a useful improvement overall. For instance, we may no longer need to stash a `it's a kernel` flag among attributes and it would probably be useful for other things (e.g enforcing address space requirements for kernel pointer arguments).



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

https://reviews.llvm.org/D68578