[PATCH] D21848: [Driver][OpenMP] Add logic for offloading-specific argument translation.

[Samuel Antao via cfe-commits]

sfantao created this revision.
sfantao added reviewers: echristo, tra, jlebar, hfinkel, ABataev, rsmith.
sfantao added subscribers: caomhin, carlo.bertolli, arpith-jacob, andreybokhanko, Hahnfeld, cfe-commits.
Herald added a subscriber: mehdi_amini.

This patch includes support for argument translation that is specific of a given offloading kind. Additionally, it implements the translation for OpenMP device kinds in the gcc tool chain.

With this patch, it is possible to compile a functional OpenMP application with offloading capabilities with no separate compilation.

http://reviews.llvm.org/D21848

Files:
include/clang/Driver/Compilation.h
include/clang/Driver/ToolChain.h
lib/Driver/Compilation.cpp
lib/Driver/Driver.cpp
lib/Driver/MSVCToolChain.cpp
lib/Driver/ToolChains.cpp
lib/Driver/ToolChains.h
test/Driver/openmp-offload.c

[Alexey Bataev via cfe-commits]

ABataev added inline comments.

================
Comment at: include/clang/Driver/Compilation.h:73
@@ +72,3 @@
+ /// architecture, and device offload kind.
+ struct TCArgsKey {
+ const ToolChain *TC;
----------------
1. 'final'
2. default initializers for fields.

================
Comment at: include/clang/Driver/Compilation.h:206
@@ +205,3 @@
+ const llvm::opt::DerivedArgList &
+ getArgsForToolChain(const ToolChain *TC, const char *BoundArch,
+ Action::OffloadKind DeviceOffloadKind);
----------------
'const' function?

================
Comment at: lib/Driver/Compilation.cpp:40
@@ -39,5 +39,3 @@
// Free any derived arg lists.
- for (llvm::DenseMap<std::pair<const ToolChain*, const char*>,
- DerivedArgList*>::iterator it = TCArgs.begin(),
- ie = TCArgs.end(); it != ie; ++it)
+ for (auto it = TCArgs.begin(), ie = TCArgs.end(); it != ie; ++it)
if (it->second != TranslatedArgs)
----------------
for(auto &Arg : TCArgs)


http://reviews.llvm.org/D21848

[Samuel Antao via cfe-commits]

sfantao updated this revision to Diff 62528.
sfantao marked 3 inline comments as done.
sfantao added a comment.

- Add default initializers to toolchain arguments key and use range based iterator in Dtor.


http://reviews.llvm.org/D21848

Files:
include/clang/Driver/Compilation.h
include/clang/Driver/ToolChain.h
lib/Driver/Compilation.cpp
lib/Driver/Driver.cpp
lib/Driver/MSVCToolChain.cpp
lib/Driver/ToolChains.cpp
lib/Driver/ToolChains.h
test/Driver/openmp-offload.c

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Alexey,

Thanks for the review!


================
Comment at: include/clang/Driver/Compilation.h:73
@@ +72,3 @@
+ /// architecture, and device offload kind.
+ struct TCArgsKey {
+ const ToolChain *TC;
----------------

Post by Alexey Bataev via cfe-commits
1. 'final'
2. default initializers for fields.

I added the default initializers but that required me to add a Ctor so that initializer lists work when this struct is built.

================
Comment at: include/clang/Driver/Compilation.h:206
@@ +205,3 @@
+ const llvm::opt::DerivedArgList &
+ getArgsForToolChain(const ToolChain *TC, const char *BoundArch,
+ Action::OffloadKind DeviceOffloadKind);
----------------

Post by Alexey Bataev via cfe-commits
'const' function?

This one can't be marked const has it potentially changes the cache of translated arguments.


http://reviews.llvm.org/D21848

[Samuel Antao via cfe-commits]

sfantao updated this revision to Diff 62583.
sfantao added a comment.

- Rebase.


http://reviews.llvm.org/D21848

Files:
include/clang/Driver/Compilation.h
include/clang/Driver/ToolChain.h
lib/Driver/Compilation.cpp
lib/Driver/Driver.cpp
lib/Driver/MSVCToolChain.cpp
lib/Driver/ToolChains.cpp
lib/Driver/ToolChains.h
test/Driver/openmp-offload.c

[Samuel Antao via cfe-commits]

sfantao updated this revision to Diff 66021.
sfantao added a comment.

- Rebase.


https://reviews.llvm.org/D21848

Files:
include/clang/Driver/Compilation.h
include/clang/Driver/ToolChain.h
lib/Driver/Compilation.cpp
lib/Driver/Driver.cpp
lib/Driver/MSVCToolChain.cpp
lib/Driver/ToolChains.cpp
lib/Driver/ToolChains.h
test/Driver/openmp-offload.c

[Samuel Antao via cfe-commits]

sfantao updated this revision to Diff 72122.
sfantao added a comment.

- Rebase.


https://reviews.llvm.org/D21848

Files:
include/clang/Driver/Compilation.h
include/clang/Driver/ToolChain.h
lib/Driver/Compilation.cpp
lib/Driver/Driver.cpp
lib/Driver/MSVCToolChain.cpp
lib/Driver/ToolChains.cpp
lib/Driver/ToolChains.h
test/Driver/openmp-offload.c

[Alexey Bataev via cfe-commits]

ABataev added a comment.

LG


https://reviews.llvm.org/D21848

[Hal Finkel via cfe-commits]

hfinkel added inline comments.

================
Comment at: lib/Driver/ToolChains.cpp:2834
@@ +2833,3 @@
+ // If this tool chain is used for an OpenMP offloading device we have to make
+ // sure we always generate a shared library regardless the commands the user
+ // passed to the host. This is required because the runtime library requires
----------------
regardless the -> regardless of the

================
Comment at: lib/Driver/ToolChains.cpp:2836
@@ +2835,3 @@
+ // passed to the host. This is required because the runtime library requires
+ // to load the device image dynamically at run time.
+ if (DeviceOffloadKind == Action::OFK_OpenMP) {
----------------
requires to load -> is required to load

================
Comment at: lib/Driver/ToolChains.cpp:2854
@@ +2853,3 @@
+ case options::OPT_shared:
+ case options::OPT_static:
+ case options::OPT_fPIC:
----------------
And also?

case options::OPT_dynamic:


https://reviews.llvm.org/D21848

[Samuel Antao via cfe-commits]

sfantao updated this revision to Diff 75732.
sfantao marked 3 inline comments as done.
sfantao added a comment.

- Fix typos and check -dynamic when it comes to translating arguments for offloading gcc toolchains.


https://reviews.llvm.org/D21848

Files:
include/clang/Driver/Compilation.h
include/clang/Driver/ToolChain.h
lib/Driver/Compilation.cpp
lib/Driver/Driver.cpp
lib/Driver/MSVCToolChain.cpp
lib/Driver/ToolChains.cpp
lib/Driver/ToolChains.h
test/Driver/openmp-offload.c

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Hal,

Thanks for the review!



================
Comment at: lib/Driver/ToolChains.cpp:2854
+ case options::OPT_shared:
+ case options::OPT_static:
+ case options::OPT_fPIC:
----------------

Post by Hal Finkel via cfe-commits
And also?

Oh, yes, that one too! Thanks!


https://reviews.llvm.org/D21848

[Hal Finkel via cfe-commits]

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

LGTM


https://reviews.llvm.org/D21848