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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
