[PATCH] D80858: [HIP] Support accessing static device variable in host code

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In addition to everyone else's concerns about generating this number randomly, it's also inherently less testable.

I'm sure there's something else you could use that's reasonably likely to be unique, like a hash of the input filename or output filename or a combination of the two.



================
Comment at: clang/include/clang/Driver/Action.h:216
   const llvm::opt::Arg &Input;
-
+  unsigned Id;
   virtual void anchor();
----------------
Allowing an arbitrary string might be more adaptable.


================
Comment at: clang/lib/AST/ASTContext.cpp:10064
       return GVA_StrongODR;
+    // Externalize device side static file-scope variable for HIP.
+    if (Context.getLangOpts().HIP && Context.getLangOpts().HIPCUID &&
----------------
This needs to be a clearer statement of why this is necessary.


================
Comment at: clang/lib/AST/ASTContext.cpp:10068
+        isa<VarDecl>(D) && cast<VarDecl>(D)->isFileVarDecl() &&
+        cast<VarDecl>(D)->getStorageClass() == SC_Static) {
+      return GVA_StrongExternal;
----------------
Are you sure this doesn't apply to e.g. local statics?  Can't you have kernel lambdas, or am I confusing HIP with another language?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1094
+      if ((VD->hasAttr<CUDADeviceAttr>() || VD->hasAttr<CUDAConstantAttr>()) &&
+          VD->isFileVarDecl() && VD->getStorageClass() == SC_Static) {
+        Out << ".hip.static." << CGM.getLangOpts().HIPCUID;
----------------
Please extract this "(device || constant) && file && static" predicate instead of repeating it in three different places.


================
Comment at: clang/lib/Driver/Action.cpp:170
+  if (!Id)
+    Id = llvm::sys::Process::GetRandomNumber();
+}
----------------
I'm sure GetRandomNumber can return 0, so this logic is faulty.

This also seems like an unfortunate intrusion of HIP-specific semantics on the rest of the driver.  Maybe HIP can generate the shared id when it's setting up and cloning the job.


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

https://reviews.llvm.org/D80858