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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9194
+    while (FirstInst != SinkTarget && DeadInstructions.contains(SinkTarget))
+      SinkTarget = SinkTarget->getPrevNode();
+
----------------
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?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104603