[PATCH] D123810: [Cuda] Add initial support for wrapping CUDA images in the new driver.

[Artem Belevich via Phabricator via llvm-commits]

tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.

Please add a TODO around the items that need further work, so we don't forget about them. Review comments tend to fade from memory.



================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:193
+  }
+  static unsigned getHashValue(const OffloadKind &Val) { return Val * 37U; }
+
----------------
Is there a particular reason for multiplying by 37? Enum values by themselves should do the job just fine. 


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:726
+
+  if (Error Err = executeCommands(*FatBinaryPath, CmdArgs))
+    return std::move(Err);
----------------
jhuber6 wrote:
> tra wrote:
> > We should have a way to pass extra options to fatbinary, too. E.g. we may want to use `--compress-all`. Also, we may need to pass through `-g` for debug builds.
> > 
> > Oh. Debug builds. Makes me wonder if cuda-gdb will be able to find GPU binaries packaged by the new driver. If it does not, it will be a rather serious problem. It would likely affect various profiling tools the same way, too. Can you give it a try?
> I was planning on implementing this stuff more generally when we get the new binary tool in D125165 landed. That will allow me to more generally put any number of command line arguments into the binary itself and fish it out here. We already support a janky version for the -Xcuda-ptxas option here, but it's a mess and I'm planning on getting rid of it. Is it okay to punt that into the future? Debug builds are another sore point I don't handle super well right now but will be addressed better with D125165. 
> 
> I haven't tested cuda-gdb, but I embed the fatbinary the same way that we do in non-rdc mode. I can read them with cuobjdump in the final executable so I'm assuming it's compatible.
>  I can read them with cuobjdump in the final executable so I'm assuming it's compatible.

We should be OK then.


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:726
+
+  if (Error Err = executeCommands(*FatBinaryPath, CmdArgs))
+    return std::move(Err);
----------------
tra wrote:
> jhuber6 wrote:
> > tra wrote:
> > > We should have a way to pass extra options to fatbinary, too. E.g. we may want to use `--compress-all`. Also, we may need to pass through `-g` for debug builds.
> > > 
> > > Oh. Debug builds. Makes me wonder if cuda-gdb will be able to find GPU binaries packaged by the new driver. If it does not, it will be a rather serious problem. It would likely affect various profiling tools the same way, too. Can you give it a try?
> > I was planning on implementing this stuff more generally when we get the new binary tool in D125165 landed. That will allow me to more generally put any number of command line arguments into the binary itself and fish it out here. We already support a janky version for the -Xcuda-ptxas option here, but it's a mess and I'm planning on getting rid of it. Is it okay to punt that into the future? Debug builds are another sore point I don't handle super well right now but will be addressed better with D125165. 
> > 
> > I haven't tested cuda-gdb, but I embed the fatbinary the same way that we do in non-rdc mode. I can read them with cuobjdump in the final executable so I'm assuming it's compatible.
> >  I can read them with cuobjdump in the final executable so I'm assuming it's compatible.
> 
> We should be OK then.
> Debug builds are another sore point I don't handle super well right now but will be addressed better with D125165

OK. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123810