Page MenuHomePhabricator

[Driver][OpenMP] Update actions builder to create unbundling action when necessary.
ClosedPublic

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

Details

Summary

Each time that offloading support is requested by the user and the input file is not a source file, an action OffloadUnbundlingAction is created to signal that the input file may contain bundles, so that the proper tool is then invoked to attempt to extract the components of the bundle. This patch adds the logic to create that action in offload action builder.

The job creation for the new action will be proposed in a separate patch.

Event Timeline

sfantao updated this revision to Diff 62259.Jun 29 2016, 11:49 AM
sfantao retitled this revision from to [Driver][OpenMP] Update actions builder to create unbundling action when necessary..
sfantao updated this object.
sfantao added reviewers: echristo, tra, jlebar, hfinkel, ABataev.
ABataev added inline comments.Jun 29 2016, 8:26 PM
include/clang/Driver/Action.h
498

'final'?

504
  1. 'final'?
  2. Default initializers for fields
include/clang/Driver/Types.h
86

'const'?

sfantao updated this revision to Diff 62558.Jul 1 2016, 3:48 PM
sfantao marked 3 inline comments as done.
  • Mark classes 'final' and set defalt initializers.

Hi Alexey,

Thanks for the review!

include/clang/Driver/Types.h
86

isSrcFile is not member of a class. It is an externally visible function of the namespace types, therefore can't mark it const.

sfantao updated this revision to Diff 62565.Jul 1 2016, 4:10 PM
  • Add Ctor dropped by mistake in the previous diff.
sfantao updated this revision to Diff 62589.Jul 1 2016, 5:30 PM
  • Rebase
sfantao updated this revision to Diff 66027.Jul 28 2016, 2:52 PM
  • Rebase.
sfantao updated this revision to Diff 72124.Sep 21 2016, 3:47 PM
  • Rebase.
hfinkel added inline comments.Sep 28 2016, 12:37 PM
include/clang/Driver/Action.h
504

Don't need 'Ty' in the name of this struct.

include/clang/Driver/Types.h
84

decided the first -> decides if the first

85

preprocessor one -> preprocessing one

lib/Driver/Driver.cpp
2105

This checks that the file needs to be preprocessed. What does preprocessing have to do with this? I don't imagine that providing a preprocessed source file as input should invoke the unbundler .

test/Driver/openmp-offload.c
309

Oh, are you using .i to indicate a bundle instead of a preprocessed file? Don't do that. Please use a different suffix -- the bundler has its own file format.

hfinkel added inline comments.Sep 28 2016, 1:03 PM
lib/Driver/Driver.cpp
2105

On second thought, this is okay. It does not make sense to have a non-bundled preprocessed source for the input there, as the host and device compilation don't share a common preprocessor state.

We do need to be careful, perhaps, about .s files (which don't need preprocessing as .S files do) -- we should probably assume that all non-bundled .s files are host assembly code.

test/Driver/openmp-offload.c
309

Never mind; this is okay too.

sfantao marked 7 inline comments as done.Oct 25 2016, 11:13 AM

Hi Hal,

Thanks for the review! Comments inlined.

include/clang/Driver/Action.h
504

Ok, using DependentActionInfo now.

lib/Driver/Driver.cpp
2105

Yes, that is what we do. If the bundler tool detects that the input is not a bundle, it assumes it is host code/bits. In either case, we still generate the unbundling tool as the driver doesn't check the contents of the files.

test/Driver/openmp-offload.c
309

Ok, there is no particular suffix to indicate a file is a bundle. The (un)bundler, however, has the machinery to detect if a given file is a bundle, it just uses the extension to understand if it is a human readable file, bitcode file, or object file, because the bundle format is different in those three cases.

sfantao updated this revision to Diff 75741.Oct 25 2016, 11:25 AM
sfantao marked 3 inline comments as done.
sfantao edited edge metadata.
  • Fix typos and use StringRef() instead of const char * to follow what the Driver does today when it comes to specify the bound architectures.
hfinkel accepted this revision.Oct 26 2016, 3:27 PM
hfinkel edited edge metadata.

LGTM

lib/Driver/Driver.cpp
2105

Please add to the comment above that if the unbundler tool detects that the input is not a bundle, it assumes it is host input.

This revision is now accepted and ready to land.Oct 26 2016, 3:27 PM
sfantao updated this revision to Diff 76064.Oct 27 2016, 11:08 AM
sfantao marked an inline comment as done.
sfantao edited edge metadata.
  • Add comment explaing that the bundler tool can detect if the input file is a bundle or not.
sfantao closed this revision.Oct 27 2016, 11:10 AM