Page MenuHomePhabricator

[TrivialDeadness] Introduce API separating two different usages
ClosedPublic

Authored by anna on Nov 26 2021, 9:36 AM.

Details

Summary

The earlier usage of wouldInstructionBeTriviallyDead is based on the
assumption that the use_count of that instruction being checked will be
zero. This patch separates the API into two different ones:

  1. The strictly conservative one where the instruction is trivially dead iff the uses are dead.
  2. The slightly relaxed form, where an instruction is dead along paths where it is not used.

The second form can be used in identifying instructions that are valid
to sink down to uses (patch to be updated in D109917).

Diff Detail

Event Timeline

anna created this revision.Nov 26 2021, 9:36 AM
anna requested review of this revision.Nov 26 2021, 9:36 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
mkazantsev added a comment.EditedNov 28 2021, 8:42 PM

I think most of this logic really belongs to isGuaranteedToTransferExecutionToSuccessor. I also think that wouldInstructionBeTriviallyDeadOnPathsWithoutUse should simply be isGuaranteedToTransferExecutionToSuccessor && !mayHaveSideEffects (maybe with filtering out terminators, ehpads and stuff like this), unless you have a counter-test.

llvm/lib/Transforms/Utils/Local.cpp
459

This also looks like it can be made a part of isGuaranteedToTransferExecutionToSuccessor.

480

Please remove this check. You may think that assume always passes execution, because it is either a NOOP or entire program is undefined.

483

This should really be a part of isGuaranteedToTransferExecutionToSuccessor. I suggest to check it instead of I->willReturn().

mkazantsev added a comment.EditedNov 28 2021, 8:46 PM

One more point is that the name wouldInstructionBeTriviallyDeadOnPathsWithoutUse is not entirely clear. Does true here mean that at least one path without uses exists, or it is something that we need to check separately?

reames requested changes to this revision.Nov 29 2021, 12:26 PM

See inline comments.

llvm/include/llvm/Transforms/Utils/Local.h
90

I'd leave off the naming suffix here. I think the original name is sufficiently clear.

(i.e. "trivial dead" continues to mean "along all paths")

98

Naming suggestion:
wouldInstructionBeTriviallyDeadAlongUnusedPath

llvm/lib/Transforms/Utils/Local.cpp
447

I would suggest an alternate implementation strategy here.

Leave the "along all paths" version unchanged.

Add the new "along unused path" version, with explicitly early returns for a couple of cases, then default to calling the "along all paths" variant.

This should be lower risk and easier to review. Then later, if you think it makes sense to invert the implementation, we can do that in a single NFC with tests for both in tree.

This revision now requires changes to proceed.Nov 29 2021, 12:26 PM
anna added inline comments.Dec 1 2021, 10:53 AM
llvm/lib/Transforms/Utils/Local.cpp
447

I really like this suggestion. It shows the "added API".

459

yes, can be done in a separate patch without increasing the scope here.

anna updated this revision to Diff 391084.Dec 1 2021, 11:05 AM

addressed review comments

anna updated this revision to Diff 391087.Dec 1 2021, 11:09 AM

rebased

anna updated this revision to Diff 391089.Dec 1 2021, 11:11 AM

updated to correct diff.

reames accepted this revision.Dec 1 2021, 12:22 PM

LGTM

This revision is now accepted and ready to land.Dec 1 2021, 12:22 PM
This revision was landed with ongoing or failed builds.Dec 3 2021, 7:10 AM
This revision was automatically updated to reflect the committed changes.