rjmccall 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()); ---------------- 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. ================ Comment at: lib/CodeGen/CGCUDANV.cpp:448 + auto HandleValue = + CtorBuilder.CreateAlignedLoad(GpuBinaryHandle, CGM.getPointerAlign()); + llvm::Constant *Zero = llvm::Constant::getNullValue(HandleValue->getType()); ---------------- 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. ================ Comment at: lib/CodeGen/CGCUDANV.cpp:452 + CtorBuilder.CreateCondBr(EQZero, IfBlock, ExitBlock); + CtorBuilder.SetInsertPoint(IfBlock); + // GpuBinaryHandle = __hipRegisterFatBinary(&FatbinWrapper); ---------------- 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. https://reviews.llvm.org/D49083 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
