[PATCH] D49083: [HIP] Register/unregister device fat binary only once

[Yaxun Liu via Phabricator via cfe-commits]

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


================
Comment at: lib/CodeGen/CGCUDANV.cpp:444
+    auto HandleValue =
+        CtorBuilder.CreateAlignedLoad(GpuBinaryHandle, CGM.getPointerAlign());
+    llvm::Constant *Zero = llvm::Constant::getNullValue(HandleValue->getType());
----------------
rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > yaxunl wrote:
> > > > rjmccall wrote:
> > > > > yaxunl wrote:
> > > > > > rjmccall wrote:
> > > > > > > Do you not need to worry about concurrency here?
> > > > > > The ctor functions are executed by the dynamic loader before the program gains the control. The dynamic loader cannot excute the ctor functions concurrently since doing that would not gurantee thread safety of the loaded program. Therefore we can assume sequential execution of ctor functions here.
> > > > > Okay.  That's worth a comment.
> > > > > 
> > > > > Is the name here specified by some ABI document, or is it just a conventional name that we're picking now?
> > > > Will add a comment for that.
> > > > 
> > > > You mean `__hip_gpubin_handle`? It is an implementation detail. It is not defined by ABI or other documentation. Since it is only used internally by ctor functions, it is not a visible elf symbol. Its name is by convention since the cuda corresponding one was named __cuda_gpubin_handle.
> > > Well, it *is* ABI, right?  It's necessary for all translation units to agree to use the same symbol here or else the registration will happen multiple times.
> > Right. I created a pull request for HIP to document this https://github.com/ROCm-Developer-Tools/HIP/pull/580/files
> Okay.  Please leave a comment here explaining that this variable's name, size, and initialization pattern are part of the HIP ABI, then.
Will do


================
Comment at: lib/CodeGen/CGCUDANV.cpp:448
+    auto HandleValue =
+        CtorBuilder.CreateAlignedLoad(GpuBinaryHandle, CGM.getPointerAlign());
+    llvm::Constant *Zero = llvm::Constant::getNullValue(HandleValue->getType());
----------------
rjmccall wrote:
> Should you just make `GpuBinaryHandle` an `Address` so that you don't have to repeat the alignment assumption over and over?
> 
> Also, you should set an alignment on the variable itself.
will do


================
Comment at: lib/CodeGen/CGCUDANV.cpp:452
+    CtorBuilder.CreateCondBr(EQZero, IfBlock, ExitBlock);
+    CtorBuilder.SetInsertPoint(IfBlock);
+    // GpuBinaryHandle = __hipRegisterFatBinary(&FatbinWrapper);
----------------
rjmccall wrote:
> When I'm generating control flow like this, I find it helpful to at least use vertical spacing to separate the blocks, and sometimes I even put all the code within a block in a brace-statement (`{ ... }`) to more clearly scope the block-local values within the block.
will do


https://reviews.llvm.org/D49083