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