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

[Artem Belevich via Phabricator via cfe-commits]

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<CXXDestructorDecl>(D) && "Use other ctor with dtor decls!");
+    assert(!D->hasAttr<CUDAGlobalAttr>() && "Use other ctor with HIP kernels!");
 
----------------
Wording inconsitency -- we're checking for `CUDAGlobalAttr` but complaining about 'HIP kernels'. Just drop 'HIP' or replace with 'GPU'?


================
Comment at: clang/include/clang/AST/GlobalDecl.h:85
+      : Value(D, unsigned(Kind)) {
+    assert(D->hasAttr<CUDAGlobalAttr>() && "Decl is not a HIP kernel!");
+  }
----------------
Ditto.


================
Comment at: clang/include/clang/AST/GlobalDecl.h:129
+           cast<FunctionDecl>(getDecl())->hasAttr<CUDAGlobalAttr>() &&
+           "Decl is not a HIP kernel!");
+    return static_cast<KernelReferenceKind>(Value.getInt());
----------------
Same wording nit.


================
Comment at: clang/include/clang/AST/GlobalDecl.h:188
+           cast<FunctionDecl>(getDecl())->hasAttr<CUDAGlobalAttr>() &&
+           "Decl is not a HIP kernel!");
+    GlobalDecl Result(*this);
----------------
Ditto.


================
Comment at: clang/lib/CodeGen/CGCUDARuntime.h:69
   virtual llvm::Function *makeModuleDtorFunction() = 0;
-
-  /// Construct and return the stub name of a kernel.
-  virtual std::string getDeviceStubName(llvm::StringRef Name) const = 0;
+  virtual std::string getDeviceSideName(const Decl *ND) = 0;
 };
----------------
Adding a descriptive comment would be great. Otherwise anyone looking at the function decl without the context of this patch will be puzzled about its meaning and purpose.

Also, perhaps the argument type should be a `NamedDecl` -- the function is not used on or useful for regular `Decl`. It will save us few casts in other places, too.


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

https://reviews.llvm.org/D68578