[PATCH] D18171: [CUDA][OpenMP] Create generic offload action

[Artem Belevich via cfe-commits]

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