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

[Artem Belevich via cfe-commits]

tra added a comment.

Few minor nits and suggestions. Other than that I'm OK with the patch.


================
Comment at: lib/Driver/Action.cpp:156
@@ +155,3 @@
+  // Propagate info to the dependencies.
+  for (unsigned i = 0; i < getInputs().size(); ++i)
+    getInputs()[i]->propagateDeviceOffloadInfo(OKinds[i], BArchs[i]);
----------------
Minor style nit -- [[ http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop | LLVM coding standard says ]] :

> ... we strongly prefer loops to be written so that they evaluate it once before the loop starts.

`for (unsigned i = 0, e = getInputs().size(); i != e; ++i)`

Please check other for loops throughout the patch.

================
Comment at: lib/Driver/Action.cpp:172-179
@@ +171,10 @@
+  for (unsigned i = 0; i < DDeps.getActions().size(); ++i) {
+    auto *A = DDeps.getActions()[i];
+    // Skip actions of empty dependences.
+    if (!A)
+      continue;
+    getInputs().push_back(A);
+    A->propagateDeviceOffloadInfo(DDeps.getOffloadKinds()[i],
+                                  DDeps.getBoundArchs()[i]);
+  }
+}
----------------
It could be rephrased as "do work if we have dependencies" and make code a bit more concise.

```
if (auto *A = DDeps.getActions()[i]) {
   getInputs().push_back(A);
   A-> propagate...
}
```

================
Comment at: lib/Driver/Action.cpp:185
@@ +184,3 @@
+    return;
+  auto *A = getInputs().front();
+  Work(A, HostTC, A->getOffloadingArch());
----------------
Please add assert to verify that getInputs() is not empty.
It may be worth doing throughout the patch as there are several places where we indexing into getInputs() result without verifying its size. It's not at all obvious from the code that it's always OK to do so.

================
Comment at: lib/Driver/Action.cpp:191-202
@@ +190,14 @@
+    const OffloadActionWorkTy &Work) const {
+  auto I = getInputs().begin();
+  auto E = getInputs().end();
+  if (I == E)
+    return;
+
+  // Skip host action
+  if (HostTC)
+    ++I;
+
+  auto TI = DevToolChains.begin();
+  for (; I != E; ++I, ++TI)
+    Work(*I, *TI, (*I)->getOffloadingArch());
+}
----------------
You may want to add an assert that I and TI are both valid within the loop.


http://reviews.llvm.org/D18171