Page MenuHomePhabricator

[LV] Remove collectTriviallyDeadInstructions, already handled by VP DCE.
ClosedPublic

Authored by fhahn on Jun 23 2022, 1:39 AM.

Details

Summary

Now that removeDeadRecipes can remove most dead recipes across a whole
VPlan, there is no need to first collect some dead instructions.
Instead removeDeadRecipes can simply clean them up.

Depends D127580.

Diff Detail

Event Timeline

fhahn created this revision.Jun 23 2022, 1:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 1:39 AM
fhahn requested review of this revision.Jun 23 2022, 1:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 1:39 AM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added inline comments.Jun 26 2022, 9:57 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8951–8952

To be close to current state, dead recipes should be removed as early as possible, but they should also clean up after optimizeInductions() and/or sinkScalarOperands() (but not after mergeReplicateRegions())? Can also run it twice, keeping the late version at the end next to removeRedundantExpandSCEVRecipes(), and issuing it early instead of the preparatory collectTriviallyDeadInstructions().

llvm/test/Transforms/LoopVectorize/X86/consecutive-ptr-uniforms.ll
101

Hmm, moving removeDeadRecipes() removed alsoPack or something?

In the case of PR40816, the loads (and phi's they feed) are also redundant and can be discarded, as LV sees through them to figure out the trip count.

llvm/test/Transforms/LoopVectorize/pointer-induction.ll
39

Avoid sinking scalar GEP into first triangle?

Perhaps worth separating the movement of removeDeadRecipes() from the removal of collectTriviallyDeadInstructions() into separate patches, to clarify the origin of these changes.

fhahn marked 3 inline comments as done.Jun 28 2022, 12:29 PM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8951–8952

I am not sure we can move it up much, as the interleave group code still accesses values via IR references. In theory we could remove a load that's never used and crash.

llvm/test/Transforms/LoopVectorize/X86/consecutive-ptr-uniforms.ll
101

I think we remove some unnecessary users earlier, hence no need to pack.

We can also remove the unused phis, will do in a late patch.

llvm/test/Transforms/LoopVectorize/pointer-induction.ll
39

I think this change is combination of moving removeDeadRecipes and removing collectTriviallyDeadInstructions, through which we end up avoiding to sink

Ayal added inline comments.Jun 28 2022, 1:36 PM
llvm/test/Transforms/LoopVectorize/pointer-induction.ll
39

So does it make sense to first only hoist removeDeadRecipes() above mergeReplicateRegions(), and then remove collectTriviallyDeadInstructions in a separate patch, isolating any such effects?

fhahn marked 4 inline comments as done.Jun 29 2022, 5:32 AM
fhahn added inline comments.
llvm/test/Transforms/LoopVectorize/pointer-induction.ll
39

I can hoist removeDeadRecipes up separately, but moving it alone won't show any changes in the existing tests unfortunately.

Ayal added inline comments.Jun 29 2022, 7:27 AM
llvm/test/Transforms/LoopVectorize/pointer-induction.ll
39

I can hoist removeDeadRecipes up separately, but moving it alone won't show any changes in the existing tests unfortunately.

So be it. Is the hoist important?

fhahn updated this revision to Diff 441034.Jun 29 2022, 8:49 AM
fhahn marked an inline comment as done.

Rebase after splitting off removeDeadRecipes movement to D128831.

fhahn marked an inline comment as done.
fhahn added inline comments.
llvm/test/Transforms/LoopVectorize/pointer-induction.ll
39

The movement is important to avoid some regressions in some of the tests, due to dead instructions blocking region merging. I managed to construct a test case and split off the movement of removeDeadRecipes to D128831

Ayal accepted this revision.Jun 30 2022, 5:16 AM

Nice clean-up!
Adding a nit for potential further clean-ups.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8539

Shall assumes be dropped as a VPlan2VPlan transformation, thereby eliminating DeadInstructions, simplifying sinking after them below, and potentially taking care of the TODO? As separate follow-up patch(es).

llvm/test/Transforms/LoopVectorize/pointer-induction.ll
39

The movement is important to avoid some regressions in some of the tests, due to dead instructions blocking region merging. I managed to construct a test case and split off the movement of removeDeadRecipes to D128831

Excellent!

This revision is now accepted and ready to land.Jun 30 2022, 5:16 AM
fhahn marked 2 inline comments as done.Jul 7 2022, 8:38 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8539

As separate follow-up patch(es).

Yes I am planning on doing this as follow up, the goal is to completely remove DeadInstructions

This revision was landed with ongoing or failed builds.Jul 7 2022, 8:40 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked an inline comment as done.