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

[John McCall via Phabricator via cfe-commits]

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 model sufficiently different  from HIP that the same implementation concept doesn't apply?


================
Comment at: clang/include/clang/AST/GlobalDecl.h:52
+    assert((!D || !isa<CXXConstructorDecl>(D)) && "Use other ctor with ctor decls!");
+    assert((!D || !isa<CXXDestructorDecl>(D)) && "Use other ctor with dtor decls!");
 
----------------
This function exists primarily to be used as a common initializer for all the constructors that don't require any of the extra fields.  (This file predates LLVM's adoption of C++14, which allows constructors to delegate to other constructors.)  That's why it asserts that it's not used with constructors or destructors.  So, two questions:

- Is there a reason this function now needs to tolerate null declarations?
- There's some subtle implicit behavior here, in that references to kernels default to `HIPKernelKind::Kernel`.  Is that reasonable, or should there be a third assertion that this function isn't used with kernel declarations?


================
Comment at: clang/include/clang/AST/GlobalDecl.h:131
 
+  operator bool() const { return getAsOpaquePtr(); }
+
----------------
`explicit`, please.


================
Comment at: clang/include/clang/AST/GlobalDecl.h:162
+               getDecl()) &&
+               !cast<FunctionDecl>(getDecl())->hasAttr<CUDAGlobalAttr>() &&
            !isa<CXXConstructorDecl>(getDecl()) &&
----------------
The indentation here seems odd.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:401
     : Context(C), Out(Out_), Structor(getStructor(D)), StructorType(Type),
       SeqID(0), AbiTagsRoot(AbiTags) { }
 
----------------
Does passing down a `GlobalDecl` everywhere allow us to remove these constructors, i.e. to eliminate the `Structor` and `StructorType` fields?


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:645
+void CXXNameMangler::mangle(GlobalDecl GD) {
+  const NamedDecl *D = dyn_cast<NamedDecl>(GD.getDecl());
   // <mangled-name> ::= _Z <encoding>
----------------
This can just be `cast`, except actually I don't think you need a cast here at all given the code below.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:818
 
-  return nullptr;
+  return GlobalDecl::getFromOpaquePtr(nullptr);
 }
----------------
There's a default constructor.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1597
+      else if (auto *DD = dyn_cast<CXXDestructorDecl>(DC))
+        DCGD = GlobalDecl(DD, Dtor_Complete);
+      else
----------------
The relevant wording from the Itanium spec here is:

> For entities in constructors and destructors, the mangling of the complete object constructor or destructor is used as the base function name, i.e. the C1 or D1 version.

But you might consider pulling this out as a helper function, something like:

```
static GlobalDecl getParentOfLocalEntity(const DeclContext *DC);
```


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1599
+      else
+        DCGD = GlobalDecl(dyn_cast<FunctionDecl>(DC));
+      mangleFunctionEncoding(DCGD);
----------------
This can still be `cast`.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1899
                                       = Template.getAsOverloadedTemplate()) {
-    mangleUnqualifiedName(nullptr, (*Overloaded->begin())->getDeclName(),
+    mangleUnqualifiedName(static_cast<NamedDecl *>(nullptr), (*Overloaded->begin())->getDeclName(),
                           UnknownArity, nullptr);
----------------
You can just pass `GlobalDecl()` here.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4986
   assert(!isa<CXXConstructorDecl>(D) && !isa<CXXDestructorDecl>(D) &&
          "Invalid mangleName() call on 'structor decl!");
 
----------------
The second assertion can just be removed now, since the GD should be carrying the right information.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1028
     else
-      MC.mangleName(ND, Out);
+      MC.mangleName(GD, Out);
   } else {
----------------
Let's see if we can make this breakdown no longer necessary, since `MangleContext::mangleName` should be capable of doing the right thing starting straight from a GD.  In fact, maybe we can remove most of the specialized mangling methods (like  `mangleCXXCtor` and `mangleCXXDtor`) from `MangleContext` completely?

Unrelatedly: there's an `Out` declared in the outermost scope, but a bunch of these scopes declare their own `Out`; could you just fix that while you're editing this function?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1041
+      llvm::raw_svector_ostream Out(Buffer);
+      Out << "__device_stub__" << II->getName();
     } else {
----------------
Is this the best way of handling this, or should `shouldMangleDeclName` return true for kernels (or at least stubs?) even in C?  Honest question.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3351
+  } else if (cast<FunctionDecl>(GD.getDecl())->hasAttr<CUDAGlobalAttr>())
+    GD = GD.getWithHIPKernelKind(LangOpts.CUDAIsDevice ? HIPKernelKind::Kernel : HIPKernelKind::Stub);
+
----------------
Should this be handled in the caller, or would that make things unreasonably difficult?


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

https://reviews.llvm.org/D68578