Page MenuHomePhabricator

[Attributor] Using liveness in other attributes.
ClosedPublic

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

Details

Summary

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

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
llvm/include/llvm/Transforms/IPO/Attributor.h
676

SmallPtrSetImpl<ReturnInst *> &

llvm/lib/Transforms/IPO/Attributor.cpp
661

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.

684

& missing

727

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.

llvm/lib/Transforms/IPO/Attributor.cpp
582

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

592

return true;

767

I'll commit these changes as a fix today.

1252

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

1587

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

1611

return getKnown() && isAssumedDead(I)

to reduce duplication of logic.

1709

// At least one new instruction encountered.

llvm/test/Transforms/FunctionAttrs/arg_returned.ll
751

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.

llvm/include/llvm/Transforms/IPO/Attributor.h
866

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

llvm/lib/Transforms/IPO/Attributor.cpp
651

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

722

Test for nullptr.

989

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

1274

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

1758

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

1762

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

llvm/test/Transforms/FunctionAttrs/liveness.ll
64

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.

llvm/lib/Transforms/IPO/Attributor.cpp
1274

Please pass this as a reference not pointer.

1764

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.

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.