Page MenuHomePhabricator

[Attributor] Using liveness in other attributes.

Authored by sstefan1 on Jul 24 2019, 2:17 PM.



Modifying other AbstractAttributes to use Liveness AA and skip dead instructions.

Diff Detail


Event Timeline

sstefan1 created this revision.Jul 24 2019, 2:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2019, 2:17 PM
jdoerfert added inline comments.Jul 24 2019, 2:38 PM
676 ↗(On Diff #211599)

SmallPtrSetImpl<ReturnInst *> &

639 ↗(On Diff #211599)

Do not modify UniqueRV and do return true to indicate this return value was handled and all is OK. The idea is, when dead then ignore the return value.

675 ↗(On Diff #211599)

& missing

715 ↗(On Diff #211599)

Move into a helper function.

sstefan1 updated this revision to Diff 211994.Jul 26 2019, 1:11 PM
  • adding instruction-wise is Assumed/Known Dead checks. This resolves FIXME in TEST 2.
  • addressing comments

    It should be noted that there is still a problem in AAReturnedValues. I will have to do a debug build, which will take quiet a lot of time, to investigate further.
sstefan1 updated this revision to Diff 212298.Jul 30 2019, 2:22 AM
  • Fixing returnedvalues issues and some minor fixes

some comments, no major problem though.

582 ↗(On Diff #212298)

Maybe move this as a member into AAIsDead with the argument beeing a ArrayRef<Instruction *> so we can reuse it

592 ↗(On Diff #212298)

return true;

767 ↗(On Diff #212298)

I'll commit these changes as a fix today.

1252 ↗(On Diff #212298)

Let's move this logic into the checkForAllCallSites call so all users benefit.

1587 ↗(On Diff #212298)

please add an assert (also above) to verify I (or BB) is in the AnchoredScope.

1611 ↗(On Diff #212298)

return getKnown() && isAssumedDead(I)

to reduce duplication of logic.

1709 ↗(On Diff #212298)

// At least one new instruction encountered.

751 ↗(On Diff #212298)

Please keep these and add the additional attribute sets instead.

sstefan1 updated this revision to Diff 212594.Jul 31 2019, 9:05 AM
  • addressing comments.
sstefan1 updated this revision to Diff 212595.Jul 31 2019, 9:07 AM
  • removed leftover comments
Harbormaster completed remote builds in B35896: Diff 212595.

Almost there. I added a last set of comments, most minor.

Could you add one more test case:

internal function foo, called 2 times but one call site is dead. We want to derive something about foo from the call site.

866 ↗(On Diff #212595)

Thanks for this generic version. Please add doxygen documentation as well.

630 ↗(On Diff #212595)

If you want LivenessAA might not be available, check for null.

702 ↗(On Diff #212595)

Test for nullptr.

965 ↗(On Diff #212595)

Comments here and elsewhere say "blocks" but check instructions.

1241 ↗(On Diff #212595)

Pass this as reference (as we always do in the Attributor).

1714 ↗(On Diff #212595)

Nit: CS->getFunction() or CS.getInstruction()->getFunction()

1718 ↗(On Diff #212595)

not return true but continue! Only this call site is OK if it is dead.

64 ↗(On Diff #212595)

remove the comment.

sstefan1 updated this revision to Diff 212787.Aug 1 2019, 5:04 AM
  • Added 1 test case, addressed comments.

Could you add one more test case:

internal function foo, called 2 times but one call site is dead. We want to derive something about foo from the call site.

Is this what you had in mind?

jdoerfert accepted this revision.Aug 1 2019, 9:31 AM

One minor thing and one change request + test. Other than that, LGTM.

1721 ↗(On Diff #212787)

Move the liveness stuff before the CallSite CS stuff. If the user is dead, it does not matter if it is a call site, a direct call site, etc., it is dead after all.

Also add a test for this please, e.g., pass the function pointer to a another unknown function or store it away but the instruction that does so is dead.

1241 ↗(On Diff #212595)

Please pass this as a reference not pointer.

This revision is now accepted and ready to land.Aug 1 2019, 9:31 AM
This revision was automatically updated to reflect the committed changes.