sfantao added a comment.
Hi Art, Richard,
Thanks for looking at this patch. I tried to address Richard concerns in the
last diff.
Let me know your thoughts,
Thanks again,
Samuel
================
Comment at: lib/Driver/Tools.cpp:308-335
@@ -349,1 +307,30 @@
+ Action::OffloadKind ActiveOffloadingKind = Action::OFK_None) {
+ SmallVector<const ToolChain *, 3> RelevantToolChains;
+ // Add the current tool chain to the relevant tool chain list if it is
+ // defined.
+ if (RegularToolChain)
+ RelevantToolChains.push_back(RegularToolChain);
+
+ // Add all the offloading tool chains associated with the current action to
+ // the relevant tool chain list. If we don't have a specific active offload
+ // kind, consider all available, otherwise consider only the active kind.
+ if (ActiveOffloadingKind == Action::OFK_None ||
+ ActiveOffloadingKind == Action::OFK_Cuda) {
+ if (JA.isHostOffloading(Action::OFK_Cuda))
+ RelevantToolChains.push_back(
+ C.getSingleOffloadToolChain<Action::OFK_Host>());
+ else if (JA.isDeviceOffloading(Action::OFK_Cuda))
+ RelevantToolChains.push_back(
+ C.getSingleOffloadToolChain<Action::OFK_Cuda>());
+ }
+
+ //
+ // TODO: Add support for other offloading programming models here.
+ //
+
+ // Apply Work on all the relevant tool chains.
+ for (const auto *TC : RelevantToolChains) {
+ assert(TC && "Undefined tool chain??");
+ Work(TC);
+ }
}
----------------
rsmith wrote:
> There's no point in building a `SmallVector` here, just directly call `Work`
> when you find a toolchain that should be used.
Ok, I'm doing as you suggest.
================
Comment at: lib/Driver/Tools.cpp:317-318
@@ +316,4 @@
+ // kind, consider all available, otherwise consider only the active kind.
+ if (ActiveOffloadingKind == Action::OFK_None ||
+ ActiveOffloadingKind == Action::OFK_Cuda) {
+ if (JA.isHostOffloading(Action::OFK_Cuda))
----------------
rsmith wrote:
> What is this `ActiveOffloadingKind` parameter for? Both values that we
> actually pass in here do the exact same thing.
`ActiveOffloadingKind` was meant to signal the offloading model in use. Its
true right now we only have CUDA kind. We are proposing the OpenMP kind in
https://reviews.llvm.org/D21843 (one of the depending patches of the patch that
inserted this modification), so the goal was to pave the way for that.
In any case, I'm removing the `ActiveOffloadingKind` check, as I am calling
`AddCudaIncludeArgs` directly in the caller of this function. Let me know if
you'd like to fix this in a different way.
================
Comment at: lib/Driver/Tools.cpp:341-346
@@ -340,8 @@
-
- if (JA.isHostOffloading(Action::OFK_Cuda))
- C.getSingleOffloadToolChain<Action::OFK_Host>()->AddCudaIncludeArgs(
- Args, CmdArgs);
- else if (JA.isDeviceOffloading(Action::OFK_Cuda))
- C.getSingleOffloadToolChain<Action::OFK_Cuda>()->AddCudaIncludeArgs(
- Args, CmdArgs);
-
----------------
rsmith wrote:
> The OFK_Host / OFK_Cuda arguments here are reversed from the other two cases.
> Is that a bug that's fixed by this change, or a bug that's introduced by this
> change? :)
>
> Either way it seems that we're missing test coverage.
As per the implementation before my change, `AddCudaIncludeArgs` should be
called on the current toolchain. The outcome of using one toolchain or the
other is similar except that for a CUDA toolchain there is an extra check. So,
we'd always get the check if producing commands for both toolchains except if
compiling with host/device-only mode. I fixed the issue here and added
regression tests for host/device only .
https://reviews.llvm.org/D22518
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
