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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 10 inline comments as done.
yaxunl added inline comments.


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


================
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 &&
----------------
rjmccall wrote:
> This needs to be a clearer statement of why this is necessary.
added comments to explain why


================
Comment at: clang/lib/AST/ASTContext.cpp:10068
+        isa<VarDecl>(D) && cast<VarDecl>(D)->isFileVarDecl() &&
+        cast<VarDecl>(D)->getStorageClass() == SC_Static) {
+      return GVA_StrongExternal;
----------------
rjmccall wrote:
> 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?
function-scope static var in a device function is only visible to the device function. Host code cannot access it, therefore no need to externalize it.


================
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;
----------------
rjmccall wrote:
> Please extract this "(device || constant) && file && static" predicate instead of repeating it in three different places.
extracted as ASTContext::shouldExternalizeStaticVar


================
Comment at: clang/lib/Driver/Action.cpp:170
+  if (!Id)
+    Id = llvm::sys::Process::GetRandomNumber();
+}
----------------
rjmccall wrote:
> 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.
Changed type of ID to string. Now empty ID means disabled. 0 is now allowed as a usual ID.

Moved initialization of ID to HIP action builder.


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

https://reviews.llvm.org/D80858