[PATCH] D47394: [OpenMP][Clang][NVPTX] Replace bundling with partial linking for the OpenMP NVPTX device offloading toolchain

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea added inline comments.


================
Comment at: include/clang/Driver/Compilation.h:312
+  /// \param skipBundler - bool value set once by the driver.
+  void setSkipOffloadBundler(bool skipBundler);
+
----------------
sfantao wrote:
> gtbercea wrote:
> > sfantao wrote:
> > > gtbercea wrote:
> > > > sfantao wrote:
> > > > > Why is this a property of the compilation and not of a set of actions referring to a given target? That would allow one to combine in the same compilation targets requiring the bundler and targets that wouldn't. 
> > > > This was a way to pass this information to the OpenMP NVPTX device toolchain.
> > > > 
> > > > Both the Driver OpenMP NVPTX toolchain need to agree on the usage of the new scheme (proposed in this patch) or the old scheme (the one that is in the compiler today).
> > > > 
> > > > 
> > > I understand, but the way I see it is that it is the toolchain that skips the bundler not the compilation. I understand that as of this patch, you skip only if there is a single nvptx target. If you have more than one target, as some tests do, some toolchains will still need the bundler. So, we are making what happens with the nvptx target dependent of other toolchains. Is this an intended effect of this patch?
> > Bundler is skipped only for the OpenMP NVPTX toolchain. I'm not sure what you mean by "other toolchain".
> Is skipped for the NVPTX toolchain if there are no "other" device toolchains requested. Say I have a working pipeline that does static linking with nvptx correctly. Then on top of that I add another device to `-fopenmp-targets`, that pipeline will now fail even for nvptx, right?
It's a choice between skipping the bundler and running the current, default mode with the bundler enabled. If targets other than NVPTX are present then we default to using the bundler for all toolchains.
There is no hybrid mode enabled where some targets use the bundler and some don't. 


================
Comment at: lib/Driver/Compilation.cpp:276
+void Compilation::setSkipOffloadBundler(bool skipBundler) {
+  skipOffloadBundler = skipBundler;
+}
----------------
sfantao wrote:
> gtbercea wrote:
> > sfantao wrote:
> > > gtbercea wrote:
> > > > sfantao wrote:
> > > > > Given the logic you have below, you are assuming this is not set to false ever. It would be wise to get an assertion here in case you end up having toolchains skipping and others don't. If that is just not supported a diagnostic should be added instead.
> > > > > 
> > > > > The convention is that local variables use CamelCase.
> > > > The checks I added in the Driver will set this flag to true if all toolchains Clang offloads to support the skipping of the bundler/unbundler for object files. Currently only NVPTX toolchain can skip the bundler/unbundler for object files so the code path in this patch will be taken only for:
> > > > 
> > > > -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda
> > > Ok, if that is the case, just add an assertion here.
> > If one of the toolchains in the list of toolchains can't skip then none of them skip. If all can skip then they all skip. What assertion would you like me to add?
> If SkipOffloadBundler is set to true you don't expect it to be set to false afterwards, right? That should be asserted.
That's correct, I can add that sure.


Repository:
  rC Clang

https://reviews.llvm.org/D47394