[PATCH] D128408: [LV] Remove collectTriviallyDeadInstructions, already handled by VP DCE.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8545
   // control flow is preserved, we should keep them.
+  SmallPtrSet<Instruction *, 4> DeadInstructions;
   auto &ConditionalAssumes = Legal->getConditionalAssumes();
----------------
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).


================
Comment at: llvm/test/Transforms/LoopVectorize/pointer-induction.ll:39
 ; CHECK:       pred.store.if:
-; CHECK-NEXT:    [[TMP11:%.*]] = getelementptr inbounds i8, i8* [[NEXT_GEP]], i64 -1
-; CHECK-NEXT:    store i8 95, i8* [[TMP11]], align 1
----------------
fhahn wrote:
> Ayal wrote:
> > fhahn wrote:
> > > Ayal wrote:
> > > > fhahn wrote:
> > > > > Ayal wrote:
> > > > > > 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.
> > > > > I think this change is combination of moving removeDeadRecipes and removing collectTriviallyDeadInstructions, through which we end up avoiding to sink
> > > > So does it make sense to first only hoist removeDeadRecipes() above mergeReplicateRegions(), and then remove collectTriviallyDeadInstructions in a separate patch, isolating any such effects?
> > > I can hoist `removeDeadRecipes` up separately, but moving it alone won't show any changes in the existing tests unfortunately. 
> > > 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?
> 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
> 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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128408/new/

https://reviews.llvm.org/D128408