This is an archive of the discontinued LLVM Phabricator instance.

[Driver][OpenMP] Add support to create jobs for bundling actions.
ClosedPublic

Authored by sfantao on Jun 29 2016, 11:57 AM.

Diff Detail

Event Timeline

sfantao updated this revision to Diff 62261.Jun 29 2016, 11:57 AM
sfantao retitled this revision from to [Driver][OpenMP] Add support to create jobs for bundling actions..
sfantao updated this object.
sfantao added reviewers: echristo, tra, jlebar, hfinkel, ABataev.
ABataev added inline comments.Jun 29 2016, 8:12 PM
include/clang/Driver/Action.h
160

'\brief'

lib/Driver/Action.cpp
134
  1. Maybe it is enough to return just StringRef, rather than std::string
  2. This can be made 'static'
lib/Driver/Tools.h
141

'final'?

sfantao updated this revision to Diff 62561.Jul 1 2016, 3:56 PM
sfantao marked 3 inline comments as done.
  • Use StringRef instead of string and mark bundler tool as final.

Hi Alexey,

Thanks for the review!

lib/Driver/Action.cpp
134

True, it can be a StringRef. This function is already marked static in its declaration.

sfantao updated this revision to Diff 62567.Jul 1 2016, 4:13 PM
  • Use StringRef instead of std::string, fix comments and mark class final.
sfantao updated this revision to Diff 62590.Jul 1 2016, 5:34 PM
  • Start static function name with caps.
sfantao updated this revision to Diff 66028.Jul 28 2016, 2:52 PM
  • Rebase.
sfantao updated this revision to Diff 72125.Sep 21 2016, 3:47 PM
  • Rebase.
hfinkel accepted this revision.Sep 28 2016, 12:44 PM
hfinkel edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 28 2016, 12:44 PM
sfantao closed this revision.Oct 27 2016, 11:14 AM
thakis added a subscriber: thakis.Dec 20 2018, 7:08 PM

Sorry about the years-later question, but what's the motivation for shelling out to an external command here? In general, LLVM tries to use a library-based approach, and LLVM went e.g. through great lengths do use an integrated assembler so clang doesn't have to shell out to one. Concretely, why isn't there a clang/lib/OffloadBundle library that clang and clang-offload-bundler both use? Does the bundling have to happen out-of-process? And if so, why isn't the process something like clang -cc1bundle instead of a separate executable? It seems weird to make clang depend on another executable next to it.

I apologize for the clueless question.

Sorry about the years-later question, but what's the motivation for shelling out to an external command here? In general, LLVM tries to use a library-based approach, and LLVM went e.g. through great lengths do use an integrated assembler so clang doesn't have to shell out to one. Concretely, why isn't there a clang/lib/OffloadBundle library that clang and clang-offload-bundler both use? Does the bundling have to happen out-of-process? And if so, why isn't the process something like clang -cc1bundle instead of a separate executable? It seems weird to make clang depend on another executable next to it.

I apologize for the clueless question.

@thakis, I don't think there is anything preventing the bundler from being driven by clang and implemented in a library. The discussions about the bundler were mostly around the bundling formats - so where it should be implemented was not as thoroughly discussed as the the formats we ended up adopting. Notwithstanding, there were a few reasons that motivated the current design:

  • The bundler should have a driver: users/developers should be able to easily pack/unpack the pieces referring to different devices. It could be a clang -cc1bundle though.
  • Offloading with many targets and different host/target combinations would require a myriad of formats that need to be used together, so they were some informal discussions where people seemed to prefer to separate this complexity from clang itself. The bundler would need some dependencies to handle object files that were not needed by clang (at least at the time).
  • The main use of the bundler is the bundling/unbundling of object files which were produced by third party tools (e.g. NVIDIA tools), so there was not a clear advantage in integrating the bundler as a library, as one needs to spawn a separate process to create its dependences anyway.