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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
