[PATCH] D18170: [CUDA][OpenMP] Create generic offload toolchains

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Art,

Thanks for the the review!


================
Comment at: include/clang/Driver/Action.h:79
@@ +78,3 @@
+    OFFLOAD_None = 0x00,
+    OFFLOAD_CUDA = 0x01,
+  };
----------------
tra wrote:
> Nit: All-caps CUDA looks weird here. _Cuda may be better choice.
> If you can shorten the prefix that would be nice, too. OK_ is already taken, though. OFK_?
Ok, sure. I'm using OFK_ instead of OFFLOAD_ as suggested.

================
Comment at: include/clang/Driver/Compilation.h:42
@@ +41,3 @@
+  /// The tool chain of the offload host.
+  const ToolChain *OffloadHostToolChain;
+
----------------
tra wrote:
> This could be generalized into yet another toolchain kind and handled the same way as offload toolchains.
Ok, I created the OFK_Host kind to designate the host toolchain.

================
Comment at: include/clang/Driver/Compilation.h:46
@@ +45,3 @@
+  /// the host has to support.
+  unsigned OffloadHostKinds;
+
----------------
tra wrote:
> This could use a more descriptive name. ActiveOffloadMask?
Using ActiveOffloadMask now.

================
Comment at: include/clang/Driver/Compilation.h:51
@@ -43,1 +50,3 @@
+  typedef std::pair<const ToolChain *, Action::OffloadKind> OffloadToolChainTy;
+  SmallVector<OffloadToolChainTy, 4> OrderedOffloadingToolchains;
 
----------------
tra wrote:
> Using std::multimap<OffloadKind, Toolchain> here would probably make specific_offload_kind_iterator unnecessary and would simplify toolchain lookup.
Ok, that makes sense. I was trying to avoid the clients of this hook to iterate over a pair - something that a map of vectors would probably do without the need of the iterator.

In any case, I did as you say as it is a more clean solution.

================
Comment at: lib/Driver/Driver.cpp:406
@@ +405,3 @@
+  // We need to generate a CUDA toolchain if any of the inputs has a CUDA type.
+  for (auto &I : Inputs)
+    // Have we founs a CUDA file? If so generate the toolchain.
----------------
tra wrote:
> llvm::any_of() could be useful here:
> 
> 
> ```
> if (llvm::any_of(Inputs, [](auto &I) { return types::isCuda(I.first)})) {
>   ...addOffloadDeviceToolchain();
> }
> ```
Ok, using `any_of` now.

================
Comment at: lib/Driver/Driver.cpp:419
@@ +418,3 @@
+  //
+  // Add support for other offloading programming models here.
+  //
----------------
tra wrote:
> TODO:
Done!

================
Comment at: lib/Driver/Driver.cpp:543
@@ -520,1 +542,3 @@
 
+  // Get the toolchains for the offloading devices, if any.
+  CreateOffloadingDeviceToolChains(*C, Inputs);
----------------
tra wrote:
> Get is somewhat misplaced here. Perhaps populate or initialize would work better.
I am using `populate` now.


http://reviews.llvm.org/D18170