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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
