jhuber6 added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6223-6224
if (IsCuda || IsHIP) {
- if (Args.hasFlag(options::OPT_fgpu_rdc, options::OPT_fno_gpu_rdc, false))
+ if (Args.hasFlag(options::OPT_fgpu_rdc, options::OPT_fno_gpu_rdc, false) ||
+ Args.hasArg(options::OPT_foffload_new_driver))
CmdArgs.push_back("-fgpu-rdc");
----------------
tra wrote:
> tra wrote:
> > jhuber6 wrote:
> > > tra wrote:
> > > > If user specifies both `-fno-gpu-rdc` and `-foffload-new-driver` we
> > > > would still enable RDC compilation.
> > > > We may want to at least issue a warning.
> > > >
> > > > Considering that we have multiple places where we may check for
> > > > `-f[no]gpu-rdc` we should make sure we don't get different ideas
> > > > whether RDC has been enabled.
> > > > I think it may make sense to provide a common way to figure it out.
> > > > Either via a helper function that would process CLI arguments or
> > > > calculate it once and save it somewhere.
> > > I haven't quite finalized how to handle this. The new driver should be
> > > compatible with a non-RDC build since we simply wouldn't embed the device
> > > image or create offloading entries. It's a little bit more difficult here
> > > since the new method is opt-in so it requires a flag. We should
> > > definitely emit a warning if both are enabled (I'm assuming there's one
> > > for passing both `fgpu-rdc` and `fno-gpu-rdc`). I'll add one in.
> > >
> > > Also we could consider the new driver *the* RDC in the future which would
> > > be the easiest. The problem is if we want to support CUDA's method of RDC
> > > considering how other build systems seem to expect it. I could see us
> > > embedding the fatbinary in the object file, even if unused, just so that
> > > cuobjdump works. However we couldn't support the generation of
> > > `__cudaRegisterFatBinary_nv....` functions because then those would cause
> > > linker errors. WDYT?
> > > I'm assuming there's one for passing both fgpu-rdc and fno-gpu-rdc
> >
> > This is not a valid assumption. The whole idea behind `-fno-something` is
> > that the options can be overridden. E.g. if the build specifies a standard
> > set of compiler options, but we need to override some of them when building
> > a particular file. We can only do so by appending to the standard options.
> > Potentially we may end up having those options overridden again. While it's
> > not very common, it's certainly possible. It's also possible to start with
> > '-fno-gpu-rdc' and then override it with `-fgpu-rdc`.
> >
> > In this case, we care about the final state of RDC specified by -f*gpu-rdc
> > options, not by the fact that `-fno-gpu-rdc` is present.
> > `Args.hasFlag(options::OPT_fgpu_rdc, options::OPT_fno_gpu_rdc, false)` does
> > exactly that -- gives you the final state. If it returns false, but we have
> > `-foffload-new-driver`, then we need a warning as these options are
> > contradictory.
> >
> > The new driver should be compatible with a non-RDC build
>
> In that case, we don't need a warning, but we do need a test verifying this
> behavior.
>
> > Also we could consider the new driver *the* RDC in the future which would
> > be the easiest.
>
> SGTM. I do not know how it all will work out in the end. Your proposed model
> makes a lot of sense, and I'm guardedly optimistic about it.
> Eventually we would deprecate RDC options, but we still need to work sensibly
> when user specifies a mix of these options.
>
>
> In that case, we don't need a warning, but we do need a test verifying this
> behavior.
>
It's possible but I don't have the logic here to do it, figured we can cross
that bridge later.
> SGTM. I do not know how it all will work out in the end. Your proposed model
> makes a lot of sense, and I'm guardedly optimistic about it.
>
So the only downsides I know of, is that we don't currently replicate CUDA's
magic to JIT RDC code (We can do this with LTO anyway), and that registering
offload entries relies on the linker defining `__start / __stop` variables,
which I don't know if linkers on Windows / MacOS provide. I'd be really
interested if someone on the LLD team knew the answer to that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120272/new/
https://reviews.llvm.org/D120272
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
