[PATCH] D69228: [LV] Generalize conditions for sinking instrs for first order recurrences.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a subscriber: gilr.
Ayal added a comment.

As revert eaff300 <https://reviews.llvm.org/rGeaff3004019f97c64c88ab76da6b25106b659b30> shows, more care must be taken to avoid having one FOR (First Order Recurring) phi sink an instruction that another FOR phi needs to keep in place. Namely, any `Previous` instruction that feeds a FOR phi across the backarc must not be allowed to sink, because that may break its dominance on other instructions who use the FOR phi. The original check against `SinkAfter.count(Previous)) // Cannot rely on dominance due to motion` addresses this concern but only partially; it works if the FOR phi that needs to sink an instruction is handled before the FOR phi that needs to keep it in place. But the two phi's could be encountered in the other order.

One way to fix this is to record every "Previous" instruction using the existing SinkAfter map, as illustrated below. Another way may be to post-process all FirstOrderRecurrences along with SinkAfter.

  diff --git a/llvm/lib/Analysis/IVDescriptors.cpp b/llvm/lib/Analysis/IVDescriptors.cpp
  index bdebd71d7f6..fd78a8335e0 100644
  --- a/llvm/lib/Analysis/IVDescriptors.cpp
  +++ b/llvm/lib/Analysis/IVDescriptors.cpp
  @@ -696,6 +696,9 @@ bool RecurrenceDescriptor::isFirstOrderRecurrence(
         SinkAfter.count(Previous)) // Cannot rely on dominance due to motion.
       return false;
   
  +  // Record that Previous must stay in place, as it must continue to dominate other instructions.
  +  SinkAfter[Previous] = nullptr;
  +
     // Ensure every user of the phi node is dominated by the previous value.
     // The dominance requirement ensures the loop vectorizer will not need to
     // vectorize the initial value prior to the first iteration of the loop.
  @@ -717,6 +720,11 @@ bool RecurrenceDescriptor::isFirstOrderRecurrence(
       if (Previous == I)
         return false;
   
  +    if (SinkAfter.count(I)) {
  +      LLVM_DEBUG(dbgs() << "LV: cannot sink instruction " << *I << " because it is involved in multiple recurrence phi's.\n");
  +      return false;
  +    }
  +
       if (DT->dominates(Previous, I)) // We already are good w/o sinking.
         return true;
   
  diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
  index 44623dd0ebc..5ca40dc3974 100644
  --- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
  +++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
  @@ -7152,7 +7152,7 @@ VPlanPtr LoopVectorizationPlanner::buildVPlanWithVPRecipes(
         // handling this instruction until after we've handled the instruction it
         // should follow.
         auto SAIt = SinkAfter.find(Instr);
  -      if (SAIt != SinkAfter.end()) {
  +      if (SAIt != SinkAfter.end() && SAIt->second) {
           LLVM_DEBUG(dbgs() << "Sinking" << *SAIt->first << " after"
                             << *SAIt->second
                             << " to vectorize a 1st order recurrence.\n");


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69228