[PATCH] D21840: [Driver][CUDA][OpenMP] Reimplement tool selection in the driver.

[Hal Finkel via cfe-commits]

hfinkel added a comment.

The naming here is a bit hard to follow, we have 'dependent action', 'dependency action', 'depending action', and I think they're all supposed to mean the same thing. Only 'dependent action' sounds right to me, can we use that universally (i.e. in all comments and names of functions and variables)?


================
Comment at: lib/Driver/Driver.cpp:2394
@@ +2393,3 @@
+    Action *CurAction = *Inputs.begin();
+    if (!CurAction->isCollapsingWithDependingActionLegal() && CanBeCollapsed)
+      return nullptr;
----------------
As a micro-optimization, check CanBeCollapsed first, then call the function:

  if (CanBeCollapsed && !CurAction->isCollapsingWithDependingActionLegal())


================
Comment at: lib/Driver/Driver.cpp:2408
@@ +2407,3 @@
+          if (!CurAction->isCollapsingWithDependingActionLegal() &&
+              CanBeCollapsed)
+            return nullptr;
----------------
  if (CanBeCollapsed && !CurAction->isCollapsingWithDependingActionLegal())

================
Comment at: lib/Driver/Driver.cpp:2415
@@ +2414,3 @@
+        CurAction = OA->getHostDependence();
+        if (!CurAction->isCollapsingWithDependingActionLegal() &&
+            CanBeCollapsed)
----------------
  if (CanBeCollapsed && !CurAction->isCollapsingWithDependingActionLegal())

================
Comment at: lib/Driver/Driver.cpp:2444
@@ +2443,3 @@
+  /// collapsed with it.
+  struct JobActionInfoTy final {
+    /// The action this info refers to.
----------------
Putting "Ty" on the end of a type name seems unusual for our code base (we generally use that for typedefs or for variables that represent types of other entities). Just JobActionInfo should be fine.

================
Comment at: lib/Driver/Driver.cpp:2474
@@ +2473,3 @@
+  const Tool *
+  attemptCombineAssembleBackendCompile(ArrayRef<JobActionInfoTy> ActionInfo,
+                                       const ActionList *&Inputs,
----------------
I don't think we need 'attempt' in the name here, just make this:

  combineAssembleBackendCompile

================
Comment at: lib/Driver/Driver.cpp:2507
@@ +2506,3 @@
+  const Tool *
+  attemptCombineAssembleBackend(ArrayRef<JobActionInfoTy> ActionInfo,
+                                const ActionList *&Inputs,
----------------
We don't need 'attempt' in the name here either.

================
Comment at: lib/Driver/Driver.cpp:2540
@@ -2473,1 +2539,3 @@
   }
+  const Tool *attemptCombineBackendCompile(ArrayRef<JobActionInfoTy> ActionInfo,
+                                           const ActionList *&Inputs,
----------------
  combineBackendCompile

================
Comment at: lib/Driver/Driver.cpp:2568
@@ +2567,3 @@
+  /// are appended to \a CollapsedOffloadAction.
+  void attemptCombineWithPreprocess(const Tool *T, const ActionList *&Inputs,
+                                    ActionList &CollapsedOffloadAction) {
----------------
combineWithPreprocessor

================
Comment at: lib/Driver/Driver.cpp:2595
@@ +2594,3 @@
+
+  /// Check if a chain of action can be combined and return the tool that can
+  /// handle the combination of actions. The pointer to the current inputs \a
----------------
action -> actions

================
Comment at: lib/Driver/Driver.cpp:2632
@@ +2631,3 @@
+
+    if (!T)
+      T = attemptCombineAssembleBackendCompile(ActionChain, Inputs,
----------------
I don't think the syntactic regularity here is helpful enough to justify this extra if. Just do:

  const Tool *T = combineAssembleBackendCompile(ActionChain, Inputs,
                                                CollapsedOffloadAction);



https://reviews.llvm.org/D21840