Page MenuHomePhabricator

[Driver][OpenMP] Build jobs for OpenMP offloading actions for targets using gcc tool chains.
ClosedPublic

Authored by sfantao on Jun 29 2016, 10:53 AM.

Details

Summary

This patch adds logic to create jobs for OpenMP offloading actions by:

  • tuning the jobs result information to use the offloading prefix even for (device) linking actions.
  • replacing the device inputs of the host linking jobs by a linker script that embed them in the right sections.

Event Timeline

sfantao updated this revision to Diff 62240.Jun 29 2016, 10:53 AM
sfantao retitled this revision from to [Driver][OpenMP] Build jobs for OpenMP offloading actions for targets using gcc tool chains..
sfantao updated this object.
sfantao updated this object.Jun 29 2016, 10:56 AM
ABataev added inline comments.Jun 29 2016, 9:21 PM
lib/Driver/Tools.cpp
302

Use llvm::sys::path::replace_extension()

sfantao updated this revision to Diff 62523.Jul 1 2016, 12:38 PM
sfantao marked an inline comment as done.
  • Use llvm::sys::path::replace_extension to deal with the extension of the linker script.

Hi Alexey,

Thanks for the review! Addressed the comments in the new diff.

sfantao updated this revision to Diff 62525.Jul 1 2016, 12:45 PM
  • Remove \brief from one comment.
sfantao updated this revision to Diff 62582.Jul 1 2016, 5:13 PM
  • Rebase.
Hahnfeld added inline comments.Jul 8 2016, 2:13 AM
lib/Driver/Driver.cpp
2853–2854

I think this has to be moved out of the if! This solves the first part of the broken separate compilation with equal triples I mentioned in D21857

lib/Driver/Tools.cpp
313

The linker script should not be written when -### is specified

sfantao updated this revision to Diff 63365.Jul 8 2016, 5:57 PM
sfantao marked an inline comment as done.
  • Bind an action to an offloading prefix even if no bound architecture is used.
mkuron added a subscriber: mkuron.Jul 26 2016, 5:28 AM
sfantao updated this revision to Diff 66020.Jul 28 2016, 2:50 PM
  • Add option to dump and test the linker script contents.
  • Rebase.
sfantao updated this revision to Diff 66189.Jul 29 2016, 3:40 PM
  • Remove duplicate keyword in linker script.
sfantao updated this revision to Diff 72121.Sep 21 2016, 3:46 PM
  • Rebase.
hfinkel added inline comments.Sep 28 2016, 12:17 PM
lib/Driver/Tools.cpp
244

recurring -> according

335

We should also say 'autogenerated' somewhere in this comment.

387

I don't see why this is needed if we have -save-temps - I think we should remove this option entirely.

sfantao updated this revision to Diff 75730.Oct 25 2016, 10:37 AM
sfantao marked 3 inline comments as done.
sfantao edited edge metadata.
  • Address Hal Finkel comments - fix comments/fix linker script comment.

Hi Hal,

Thanks for the review! Comments inlined.

lib/Driver/Tools.cpp
335

Ok, makes sense. The comment is now:

      OpenMP Offload Linker Script.
*** Automatically generated by clang ***
387

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?

hfinkel accepted this revision.Oct 26 2016, 3:24 PM
hfinkel edited edge metadata.

LGTM

lib/Driver/Tools.cpp
335

Please capitalize Clang (it is capitalized in our documentation).

387

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.

This revision is now accepted and ready to land.Oct 26 2016, 3:24 PM
sfantao updated this revision to Diff 76061.Oct 27 2016, 10:33 AM
sfantao marked 2 inline comments as done.
sfantao edited edge metadata.
  • Capitalize Clang in linker script comment and explain that the linker script dump option is required to test the driver with -###.
sfantao updated this revision to Diff 76062.Oct 27 2016, 10:38 AM
  • Capitalize Clang in the regression test too.
sfantao closed this revision.Oct 27 2016, 10:40 AM