tra added a comment.
Thank you for making these changes. They don't solve all the shortcomings, but
they improve things quite a bit, IMO.
Overall I'm happy with the changes, though use of mutable and changing action
state from const functions may need a look from someone with better C++-fu
skills than myself.
@jlebar: Justin, any suggestions on what we can/should do regarding that?
================
Comment at: include/clang/Driver/Action.h:95
@@ +94,3 @@
+ /// same host. Therefore, the host offloading kind is a combination of kinds.
+ mutable unsigned OffloadingHostKind;
+ /// \brief Offloading kind of the device.
----------------
I assume that by combination you imply that this is a mask of all offloading
kinds we intend to use.
Perhaps it should be renamed to reflect it better. ActiveOffloadKindMask?
================
Comment at: include/clang/Driver/Action.h:95
@@ +94,3 @@
+ /// same host. Therefore, the host offloading kind is a combination of kinds.
+ mutable unsigned OffloadingHostKind;
+ /// \brief Offloading kind of the device.
----------------
tra wrote:
> I assume that by combination you imply that this is a mask of all offloading
> kinds we intend to use.
> Perhaps it should be renamed to reflect it better. ActiveOffloadKindMask?
These fields appears to be part of the Action state yet you want to modify them
from const functions which seems wrong to me. Perhaps functions that set these
fields should not be const.
================
Comment at: include/clang/Driver/Action.h:137
@@ +136,3 @@
+ /// dependences.
+ void propagateDeviceOffloadInfo(OffloadKind OKind, const char *OArch) const;
+ /// \brief Append the host offload info of this action and propagate it to
its
----------------
propagateXXX implies modification which contradicts 'const' which implies that
there will be no state change. Which one is telling the truth?
================
Comment at: include/clang/Driver/Action.h:208-215
@@ +207,10 @@
+ private:
+ /// \brief The dependence action.
+ ActionList AL;
+ /// \brief The offloading toolchains that should be used with the action.
+ SmallVector<const ToolChain *, 3> TCL;
+ /// \brief The architectures that should be used with this action.
+ SmallVector<const char *, 3> BAL;
+ /// \brief The offload kind of each dependence.
+ SmallVector<OffloadKind, 3> KL;
+
----------------
Are these independent or is it expected that all four are populated/modified in
sync? If they all need to be updated at the same time (which is what the code
below does), it should be documented. Perhaps these arrays could be converted
into vector of structs.
================
Comment at: include/clang/Driver/Action.h:220
@@ +219,3 @@
+ /// offload kind.
+ void add(Action *A, const ToolChain *TC, const char *BoundArch,
+ OffloadKind OKind);
----------------
Can any of parameters be nullptr?
================
Comment at: include/clang/Driver/Action.h:234-240
@@ +233,9 @@
+ /// \brief The dependence action.
+ Action *A;
+ /// \brief The offloading toolchain that should be used with the action.
+ const ToolChain *TC;
+ /// \brief The architectures that should be used with this action.
+ const char *BoundArch;
+ /// \brief The offload kind of each dependence.
+ unsigned OffloadKinds;
+
----------------
Cosmetic nit: Naming is somewhat inconsistent. Device dependencies use
acronyms, but HostDependence uses mix of acronyms and words. I'd use camelcase
words in both.
================
Comment at: include/clang/Driver/Driver.h:422
@@ -424,1 +421,3 @@
+ bool AtTopLevel, bool MultipleArchs,
+ const ToolChain *TC) const;
----------------
TC is only used to get triple to construct file name prefix. Perhaps just pass
that string explicitly.
================
Comment at: lib/Driver/Action.cpp:44
@@ +43,3 @@
+void Action::propagateDeviceOffloadInfo(OffloadKind OKind,
+ const char *OArch) const {
+ // Offload action set its own kinds on their dependences.
----------------
Given that these functions change state they probably should not be const.
================
Comment at: lib/Driver/Action.cpp:103
@@ +102,3 @@
+
+std::string Action::getOffloadingFileNamePrefix(const ToolChain *TC) const {
+ // A file prefix is only generated for device actions and consists of the
----------------
There's no need for ToolChain here. A string is all it needs as an input.
================
Comment at: lib/Driver/Action.cpp:208
@@ +207,3 @@
+Action *OffloadAction::getSingleDeviceDependence() const {
+ return (!HostTC && getInputs().size() == 1) ? getInputs().front() : nullptr;
+}
----------------
returning nullptr if .size() != 1 looks strange. Perhaps there should be an
assert.
================
Comment at: lib/Driver/Driver.cpp:1391-1395
@@ -1357,5 +1390,7 @@
+ const ToolChain *CudaTC =
+ C.getSingleOffloadDeviceToolChain<Action::OFFLOAD_CUDA>();
+
// Build actions for all device inputs.
- assert(C.getSingleOffloadDeviceToolChain<Action::OFFLOAD_CUDA>() &&
- "Missing toolchain for device-side compilation.");
+ assert(CudaTC && "Missing toolchain for device-side compilation.");
ActionList CudaDeviceActions;
----------------
Perhaps this should be moved down closer to where it's used. Perhaps even
inside of if(PartialCompilation ...)
================
Comment at: lib/Driver/Driver.cpp:1464
@@ +1463,3 @@
+ OffloadAction::DeviceDependences DDep;
+ DDep.add(FatbinAction, CudaTC, /*BoundArch=*/nullptr, Action::OFFLOAD_CUDA);
+ return C.MakeAction<OffloadAction>(HDep, DDep);
----------------
Is toolchain needed for fatbin action?
================
Comment at: lib/Driver/Driver.cpp:1884
@@ +1883,3 @@
+ }
+ if (auto *DDep = OA->getSingleDeviceDependence())
+ if (isa<T>(DDep)) {
----------------
You could fold both ifs into something like this:
```
if (auto *DDAP = dyn_cast_or_null<T>(OA->getSingleDeviceDependence()))
```
================
Comment at: lib/Driver/Driver.cpp:2062
@@ +2061,3 @@
+ // If we have a single device action, just return its info.
+ if (!HostAction && OffloadDeviceInputInfos.size() == 1) {
+ return OffloadDeviceInputInfos.back();
----------------
It may be worth adding a comment explaining what happens if
OffloadDeviceInputInfos.size() != 1.
================
Comment at: lib/Driver/Tools.cpp:3595
@@ -3594,3 +3594,3 @@
CmdArgs.push_back("-aux-triple");
CmdArgs.push_back(Args.MakeArgString(AuxToolChain->getTriple().str()));
CmdArgs.push_back("-fcuda-target-overloads");
----------------
All we need is a target triple here. Now that we have device offloading info,
perhaps we can bypass AuxToolchain and let offloading info provide host or
device triple directly. That would render FIXME above obsolete, IMO.
http://reviews.llvm.org/D18171
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
