[PATCH] D21847: [Driver][OpenMP] Build jobs for OpenMP offloading actions for targets using gcc tool chains.

[Hal Finkel via cfe-commits]

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

LGTM



================
Comment at: lib/Driver/Tools.cpp:334
+  LksStream << "  OpenMP Offload Linker Script.\n";
+  LksStream << "*/\n";
+  LksStream << "TARGET(binary)\n";
----------------
sfantao wrote:
> hfinkel wrote:
> > We should also say 'autogenerated' somewhere in this comment.
> Ok, makes sense. The comment is now:
> ```
>                    OpenMP Offload Linker Script.
>              *** Automatically generated by clang ***
> ```
Please capitalize Clang (it is capitalized in our documentation).


================
Comment at: lib/Driver/Tools.cpp:386
+  // Dump the contents of the linker script if the user requested that.
+  if (C.getArgs().hasArg(options::OPT_fopenmp_dump_offload_linker_script))
+    llvm::errs() << LksBuffer;
----------------
sfantao wrote:
> hfinkel wrote:
> > I don't see why this is needed if we have -save-temps - I think we should remove this option entirely.
> The reason for adding this option is that the test is done when the driver is in dry-run mode (`-###`) so I'm not supposed to generate any files. If we don't run in dry-run mode, we need to allow linking to actually happen, therefore the machine where the tests runs needs to have a gcc-based toolchain and ld. 
> 
> Is there a way to request that in the required features set in llvm-lit config file? Should I add a new feature?
Okay, I understand. Please add to the comment that this is used so that we can test test behavior  with -###. I don't think that we want the regression tests to really call the system linker, etc.


https://reviews.llvm.org/D21847