This is an archive of the discontinued LLVM Phabricator instance.

[Driver][CUDA][OpenMP] Reimplement tool selection in the driver.
ClosedPublic

Authored by sfantao on Jun 29 2016, 9:45 AM.

Details

Summary

This creates a tool selector in the driver that replaces the existing one. The goal is to better organize the code and make the selector easier to scale, in particular in the presence of offload actions that can be collapsed.

The current implementation became more confusing when the support for offloading actions was added. This concern was expressed by Eric in http://reviews.llvm.org/D9888.

This patch does not add new testing, it preserves the existing functionality.

Diff Detail

Event Timeline

sfantao updated this revision to Diff 62232.Jun 29 2016, 9:45 AM
sfantao retitled this revision from to [Driver][CUDA][OpenMP] Reimplement tool selection in the driver..
sfantao updated this object.
mkuron added a subscriber: mkuron.Jun 29 2016, 12:57 PM
ABataev edited edge metadata.Jun 29 2016, 7:57 PM

General comment: remove '\brief' tags, they are not required anymore, just '\\\' is enough

include/clang/Driver/Action.h
94

'\brief' tag is not required, remove it.

lib/Driver/Driver.cpp
2396

bool canCollapseAssembleAction() -> bool canCollapseAssembleAction() const

2404

Also can be marked as const

2412

I think this can be marked 'final' and add 'nullptr' as a default initializer for 'JA' field

2420

Remove '\brief'

2422

I think this function can be marked as 'static', no?

2431

remove '\brief'

sfantao updated this revision to Diff 62507.Jul 1 2016, 10:26 AM
sfantao marked 7 inline comments as done.
sfantao edited edge metadata.
  • Fix comments.
  • Mark functions properly with const and static.
  • Remove \brief.

Hi Alexey,

Thanks for the review!

lib/Driver/Driver.cpp
2412

Yep, I'm doing that now.

2422

You're right, using static now.

sfantao updated this revision to Diff 62575.Jul 1 2016, 5:03 PM
  • Rebase
sfantao updated this revision to Diff 63682.Jul 12 2016, 8:17 AM
  • Rebase.
sfantao updated this revision to Diff 66017.Jul 28 2016, 2:49 PM
  • Rebase.
sfantao updated this revision to Diff 72118.Sep 21 2016, 3:46 PM
  • Rebase.
hfinkel edited edge metadata.Sep 28 2016, 11:41 AM

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

lib/Driver/Driver.cpp
2362

As a micro-optimization, check CanBeCollapsed first, then call the function:

if (CanBeCollapsed && !CurAction->isCollapsingWithDependingActionLegal())
2376
if (CanBeCollapsed && !CurAction->isCollapsingWithDependingActionLegal())
2383
if (CanBeCollapsed && !CurAction->isCollapsingWithDependingActionLegal())
2412

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.

2442

I don't think we need 'attempt' in the name here, just make this:

combineAssembleBackendCompile
2474

We don't need 'attempt' in the name here either.

2507
combineBackendCompile
2535

combineWithPreprocessor

2562

action -> actions

2599

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);
sfantao updated this revision to Diff 75698.Oct 25 2016, 7:25 AM
sfantao marked 10 inline comments as done.
sfantao edited edge metadata.
  • Address Hal Finkel suggestions - rename functions/reorder code/fix comments.

Hi Hal,

Thanks for the review!

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

lib/Driver/Driver.cpp
2362

Ok, makes sense. Fixed this in the last diff.

2412

Ok, fixed that in the last diff.

2442

Ok, fixed in last diff.

2599

Ok, fixed in last diff.

hfinkel accepted this revision.Oct 26 2016, 3:09 PM
hfinkel edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 26 2016, 3:09 PM
sfantao closed this revision.Oct 27 2016, 9:38 AM