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

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Hal,

Thanks for the review!

In https://reviews.llvm.org/D21840#555719, @hfinkel wrote:

> 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)?


I agree the Depending/Dependence stuff can be confusing. However, I tried to use Depending and Dependence to indicate different things:

- Depending action -> an action that depends on the current one
- Dependence action ->  an action that is a dependence to the current one

Of course they all are dependent actions, so your suggestion definitely makes sense. So, in the last diff I indicate:

- Depending action -> Next Dependent action
- Dependence action -> Prev(ious) Dependent action

I hope this helps clarifying things. Let me know you thoughts.

Thanks again!
Samuel



================
Comment at: lib/Driver/Driver.cpp:2394
+    Action *CurAction = *Inputs.begin();
+    if (!CurAction->isCollapsingWithDependingActionLegal() && CanBeCollapsed)
+      return nullptr;
----------------
hfinkel wrote:
> As a micro-optimization, check CanBeCollapsed first, then call the function:
> 
>   if (CanBeCollapsed && !CurAction->isCollapsingWithDependingActionLegal())
> 
Ok, makes sense. Fixed this in the last diff.


================
Comment at: lib/Driver/Driver.cpp:2444
+  /// collapsed with it.
+  struct JobActionInfoTy final {
+    /// The action this info refers to.
----------------
hfinkel wrote:
> 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.
Ok, fixed that in the last diff.


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


================
Comment at: lib/Driver/Driver.cpp:2632
+
+    if (!T)
+      T = attemptCombineAssembleBackendCompile(ActionChain, Inputs,
----------------
hfinkel wrote:
> 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);
> 
Ok, fixed in last diff.


https://reviews.llvm.org/D21840