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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

In D104603#2831278 <https://reviews.llvm.org/D104603#2831278>, @fhahn wrote:

> In D104603#2830492 <https://reviews.llvm.org/D104603#2830492>, @Ayal wrote:
>
>> Ahh, the culprit here is a chain of FOR phi-users all sinking after a Previous - the latter should never be dead, but any member of the chain may by dead.
>> In the test case: `%rec.2` is the FOR phi, `%rec.2.prev` is Previous, and {`%cmp`, `%C`, `%B2`} is the chain of users, where `%B2` is dead as it feeds the loop's conditional branch only.
>
> Yes exactly! Let me improve the naming of the variables a bit.

Good! Can also call out explicitly the %dead value.

>> An alternative fix is to have RecurrenceDescriptor::isFirstOrderRecurrence() sink the chain in reverse, always after the non-dead Previous; as you originally suggested in D84951 <https://reviews.llvm.org/D84951>?
>
> I think another alternative would be to handle the check for dead instructions directly at the place where we do sinking. I updated the code to do that. Please let me know if you prefer the alternative.
>
> I think chaining the instructions in the map makes sense and the handling of dead instructions before VPlan creation may go away in the medium term anyways?

Agreed that in the medium term, dead instructions may be handled differently in/before VPlan/Legal, so would be good to have a simple fix for now.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9029
   for (Instruction *I : DeadInstructions)
     SinkAfter.erase(I);
 
----------------
It may be better to handle all updates of SinkAfter due to DeadInstructions in one place, i.e., erasing dead sinks and bumping dead targets, either both here or both below inside buildVPlanWithVPRecipes(Range).


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104603