Page MenuHomePhabricator

clang-offload-bundler - offload files bundling/unbundling tool
ClosedPublic

Authored by sfantao on Oct 20 2015, 11:53 AM.

Details

Summary

One of the goals of programming models that support offloading (e.g. OpenMP) is to enable users to offload with little effort, by annotating the code with a few pragmas. I'd also like to save users the trouble of changing their existent applications' build system. So having the compiler always return a single file instead of one for the host and each target even if the user is doing separate compilation is desirable.

This diff proposes a tool named clang-offload-bundler (happy to change the name if required) that is used to bundle files associated with the same user source file but different targets, or to unbundle a file into separate files associated with different targets.

This tool supports the driver support for OpenMP under review in http://reviews.llvm.org/D9888. The tool is used there to enable separate compilation, so that the very first action on input files that are not source files is a "unbundling action" and the very last non-linking action is a "bundling action".

The format of the bundled files is currently very simple: text formats are concatenated with comments that have a magic string and target identifying triple in between, and binary formats have a header that contains the triple and the offset and size of the code for host and each target.

The goal is to improve this tool in the future to deal with archive files so that each individual file in the archive is properly dealt with. We see that archives are very commonly used in current applications to combine separate compilation results. So I'm convinced users would enjoy this feature.

This tool can be used like this:

clang-offload-bundler -targets=triple1,triple2 -type=ii -inputs=a.triple1.ii,a.triple2.ii -outputs=a.ii

or

clang-offload-bundler -targets=triple1,triple2 -type=ii -outputs=a.triple1.ii,a.triple2.ii -inputs=a.ii -unbundle

I implemented the tool under clang/tools. Please let me know if something like this should live somewhere else.

This patch is prerequisite for http://reviews.llvm.org/D9888.

Diff Detail

Event Timeline

sfantao updated this revision to Diff 37901.Oct 20 2015, 11:53 AM
sfantao retitled this revision from to clang-offload-bundler - offload files bundling/unbundling tool.
sfantao updated this object.
sfantao added a subscriber: cfe-commits.
sfantao updated this revision to Diff 39590.Nov 6 2015, 2:18 PM

Rebase.

sfantao updated this revision to Diff 40999.Nov 23 2015, 5:12 PM
sfantao updated this object.

Rebase.

sfantao updated this revision to Diff 62249.Jun 29 2016, 11:10 AM
  • Reorganize file handlers. Generate empty files if no components exist in the bundle instead of just failing.
ABataev edited edge metadata.Jun 29 2016, 9:17 PM

Remove empty 'return' and ';' statements where they are not required.

test/Driver/clang-offload-bundler.c
15

Hmm, will it work on Windows? Maybe it is better just to add an empty test file?

tools/clang-offload-bundler/ClangOffloadBundler.cpp
175
  1. 'final'
  2. Default field initializers instead of default constructor
305–306

remove empty 'return's

309

Remove it

336

Default initializer

454

auto &File instead of auto I?

478–479

for (auto &Triple : TargetNames)

514–515

for (auto &Triple : TargetNames)

678–681

Maybe 'return Unbundle ? UnbundleFiles() : BundleFiles();'?

sfantao updated this revision to Diff 62538.Jul 1 2016, 2:06 PM
sfantao marked 9 inline comments as done.
sfantao edited edge metadata.
  • Merge branch 'master' into patch-D13909
  • Remove unecessary returns and fix iterators.

Hi Alexey,

Thanks for the review!

test/Driver/clang-offload-bundler.c
15

I see many other tests in clang using it. There used to be a directive //requires-shell but I don't think that is being used now. Should I go ahead and create an empty file? Unfortunately, it is not easy for me to try these things on Windows.

tools/clang-offload-bundler/ClangOffloadBundler.cpp
478–479

Ok, moved the iterator Input to the end of the loop body.

514–515

Ok, moved Output iterator to the bottom of the loop body.

sfantao updated this revision to Diff 62540.Jul 1 2016, 2:12 PM
  • Remove \brief when not needed.
sfantao updated this revision to Diff 63362.Jul 8 2016, 5:24 PM
  • Add tests for when the host does not come first.
  • Fix format of comments and add variable to trace the position of the host input.
sfantao updated this revision to Diff 66023.Jul 28 2016, 2:51 PM
  • Fix bug in conditional.
  • Rebase.
Hahnfeld accepted this revision.Aug 12 2016, 1:46 AM
Hahnfeld added a reviewer: Hahnfeld.

LGTM with only some minor nits on some of the comments and a CMake question

test/CMakeLists.txt
27–33

I think clang-offload-bundler needs to be added as dependency for the clang target because it will really need the bundler at runtime, not only when testing...

(Disclaimer: I'm no CMake expert)

tools/clang-offload-bundler/ClangOffloadBundler.cpp
103–105

s/bundled/bundle/?

152

to/from?

165

Duplicate and write? to/from?

569

Better say that we haven't found the bundle for the host?

This revision is now accepted and ready to land.Aug 12 2016, 1:46 AM
sfantao updated this revision to Diff 68086.Aug 15 2016, 3:01 PM
sfantao marked 5 inline comments as done.
sfantao edited edge metadata.
  • Fix comments and diagnostics.

Hi Jonas,

Thanks for the review!

test/CMakeLists.txt
27–33

The bundler tool already depends on clang, so that would cause a circular dependency. I think that in general not building the bundler is fine - the user may not be interested in doing offloading, so if he attempts to do so, that would fail as, say, ld was not in the system.

I'm adding it only for testing because there are tests that will exercise the bundler that will fail if the driver does not detect the tool.

Should we ask someone in specific for an opinion? Let me know your thoughts.

tools/clang-offload-bundler/ClangOffloadBundler.cpp
152

Thanks for catching this! Fixed in the last diff, it should be from.

165

Fixed in the last diff, it should be to.

569

Makes sense, I changed the message in the last diff.

Hahnfeld added inline comments.Aug 16 2016, 12:55 AM
test/CMakeLists.txt
27–33

Most users will get it anyway because it is built for the install target and I think the build system should do its best to build and install all needed dependencies.

I think this currently only fails when using make clang and then trying to invoke the compiler from the build directory. I agree that this should be quite rare but that could be fixed by add_dependencies(clang clang-offload-bundler) which has worked for me. However I now don't have a really strong opinion here because it works with the install target.

sfantao updated this revision to Diff 69124.Aug 24 2016, 8:23 AM
sfantao marked an inline comment as done.
  • Add clang-offload bundler as dependency to clang.

Hi Jonas,

Thanks again for the review!

test/CMakeLists.txt
27–33

Ok, I added the line add_dependencies(clang clang-offload-bundler) as you suggest. I was worried that referring to clang libs would cause a circular dependency, but it seems to work just fine.

sfantao closed this revision.Aug 24 2016, 8:29 AM