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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
