[PATCH] D104603: [LV] Fix crash when target instruction for sinking is dead.

[Ayal Zaks via Phabricator via llvm-commits]

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

Thanks!



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9043
+          SinkTarget != FirstInst &&
+          "Must find a alive instruction (at least the one feeding the "
+          "first-order recurrence PHI) before reaching beginning of the block");
----------------
a alive >> a live


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9045
+          "first-order recurrence PHI) before reaching beginning of the block");
+      assert(SinkTarget != P.first &&
+             "sink source equals target, no sinking required");
----------------
This second assert(SinkTarget != P.first) should be placed right after bumping SinkTarget to its prev node.
(We know P.first ain't dead - these were taken care of above; so if SinkTarget get set to P.first, we will immediately exit the loop, before reaching this assert here. So can also place the assert right after the loop.)
(The first assert is placed well, guarding against taking FirstInst->getPrevNode().)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9194
+    while (FirstInst != SinkTarget && DeadInstructions.contains(SinkTarget))
+      SinkTarget = SinkTarget->getPrevNode();
+
----------------
fhahn wrote:
> Ayal wrote:
> > Traversing the dead backwards here should also be ok, but raises additional thoughts.
> > If we reach FirstInst == SinkTarget then no sinking is required, which should not happen, so best have an assert instead? May want to also assert against falling off the first recipe of the block.
> > 
> > Additional alternatives, on top of reversing SinkAfter chains, include (mainly for completion):
> > 
> >   # Letting Legal handle DeadInstructions, so that SinkTarget will include only live sinks to be moved after live targets. But there's a dependence on FoldTail wanting to "reuse" Primary Induction.
> >   # Having the first VPlan wrap Dead Instructions with a (recorded) Dead Recipe, to be eliminated by a VPlan-to-VPlan DCE.
> >   # Set LastLiveSecond = (Entry.second is live ? Entry.second : LastLiveSecond), and always sink SinkTarget after LastLiveSecond. This relies on iteration order over SinkAfter being {U_1,Previous},{U_2,U_1},...,{U_k,U_{k-1}} whenever some target U_i is dead, where Previous must be live.
> > 
> > Can also handle dead sinks here, as mentioned above, e.g., by `continue`. Less efficient, but sizes of SinkAfter and DeadInstructions should be very small if positive.
> > 
> > Can also update the sinkAfter pair whose target was revived, caching the result for next Range. 
> > Traversing the dead backwards here should also be ok, but raises additional thoughts.
> > If we reach FirstInst == SinkTarget then no sinking is required, which should not happen, so best have an assert instead? May want to also assert against falling off the first recipe of the block.
> 
> I added 2 assertions above:
> 
> * `SinkTarget != FirstInst`, should guard against failing of the first recipe in the block
> * `SinkTarget != SinkSource`, should guard against cases not requiring any sinking?
> 
> WDYT?
> 
> 
+1, just a remark above about where to place the second assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104603