[PATCH] D134083: [IVDescriptors] Before moving an instruction in SinkAfter checking if it is target of other instructions

[Guozhi Wei via Phabricator via llvm-commits]

Carrot created this revision.
Carrot added reviewers: fhahn, tvvikram.
Herald added subscribers: rogfer01, bollu, hiraditya.
Herald added a project: All.
Carrot requested review of this revision.
Herald added subscribers: llvm-commits, pcwang-thead, vkmr.
Herald added a project: LLVM.

The attached test case can cause LLVM crash in buildVPlanWithVPRecipes because invalid VPlan is generated.

  FIRST-ORDER-RECURRENCE-PHI ir<%792> = phi ir<%501>, ir<%806>
  CLONE ir<%804> = fdiv ir<1.000000e+00>, vp<%17>      // use of %17
  CLONE ir<%806> = load ir<%805>
  EMIT vp<%17> = first-order splice ir<%792> ir<%806>   // def of %17
  ...

There is a use before def error on %17.

When vectorizer generates a VPlan, it generates a "first-order splice" instruction for a loop carried variable after its definition. All related PHI users are changed to use this "first-order splice" result, and are moved after it. The move is guided by a MapVector SinkAfter. And the content of SinkAfter is filled by RecurrenceDescriptor::isFixedOrderRecurrence.

  Let's look at the first PHI and related instructions
  
  %v792 = phi double [ %v806, %Loop ], [ %d1, %Entry ]
  %v802 = fdiv double %v794, %v792
  %v804 = fdiv double 1.000000e+00, %v792
  %v806 = load double, ptr %v805, align 8
  
  %v806 is a loop carried variable, %v792 is related PHI instruction. 
  Vectorizer will generated a new "first-order splice" instruction 
  for %v806, and it will be used by %v802 and %v804. So %v802 and %v804 
  will be moved after %v806 and its "first-order splice" instruction.
  So SinkAfter contains
  
     %v802   ->  %v806
     %v804   ->  %v802
  
  It means %v802 should be moved after %v806 and %v804 will be moved 
  after %v802. Please pay attention that the order is important.
  
  When isFixedOrderRecurrence processing PHI instruction %v794, related
  instructions are
  
    %v793 = phi double [ %v813, %Loop ], [ %d1, %Entry ]
    %v794 = phi double [ %v793, %Loop ], [ %d2, %Entry ]
    %v802 = fdiv double %v794, %v792
    %v813 = load double, ptr %v812, align 8
  
  This time its related loop carried variable is %v813, its user is %v802.
  So %v802 should also be moved after %v813. But %v802 is already in 
  SinkAfter, because %v813 is later than %v806, so the original %v802 entry
  in SinkAfter is deleted, a new %v802 entry is added. Now SinkAfter contains
  
     %v804   ->  %v802
     %v802   ->  %v813
  
  With these data, %v802 can still be moved after all its operands, but %v804
  can't be moved after %v806 and its "first-order splice" instruction. And 
  causes use before def error.

So when remove/re-insert an instruction I in SinkAfter, we should also recursively remove instructions targeting I and re-insert them into SinkAfter. But for simplicity I just bail out in this case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134083

Files:
  llvm/lib/Analysis/IVDescriptors.cpp
  llvm/test/Transforms/LoopVectorize/sinkafter.ll


Index: llvm/test/Transforms/LoopVectorize/sinkafter.ll
===================================================================
--- /dev/null
+++ llvm/test/Transforms/LoopVectorize/sinkafter.ll
@@ -0,0 +1,36 @@
+; RUN: opt -loop-vectorize -S %s | FileCheck %s
+
+; Make sure LLVM doesn't generate wrong data in SinkAfter, and causes crash in
+; loop vectorizer.
+
+; CHECK-LABEL: @foo
+; CHECK:       ret
+define void @foo(i64 %count, double %d1, double %d2, ptr %p1, ptr %p2) {
+Entry:
+  br label %Loop
+
+Loop:
+  %v792 = phi double [ %v806, %Loop ], [ %d1, %Entry ]
+  %v793 = phi double [ %v813, %Loop ], [ %d1, %Entry ]
+  %v794 = phi double [ %v793, %Loop ], [ %d2, %Entry ]
+  %v795 = phi i64 [ %v811, %Loop ], [ 1, %Entry ]
+  %v802 = fdiv double %v794, %v792
+  %v803 = getelementptr inbounds [4 x double], ptr %p1, i64 %v795
+  %v804 = fdiv double 1.000000e+00, %v792
+  %v805 = getelementptr inbounds double, ptr %p2, i64 %v795
+  %v806 = load double, ptr %v805, align 8
+  %v807 = fdiv double 1.000000e+00, %v806
+  %v808 = fadd double %v804, %v807
+  %v809 = fmul double %v793, %v808
+  %v810 = fsub double %v802, %v809
+  %v811 = add nuw nsw i64 %v795, 1
+  %v812 = getelementptr inbounds [4 x double], ptr %p1, i64 %v811, i64 2
+  %v813 = load double, ptr %v812, align 8
+  %v815 = fadd double %v813, %v810
+  store double %v815, ptr %v803, align 8
+  %v818 = icmp eq i64 %v811, %count
+  br i1 %v818, label %End, label %Loop
+
+End:
+  ret void
+}
Index: llvm/lib/Analysis/IVDescriptors.cpp
===================================================================
--- llvm/lib/Analysis/IVDescriptors.cpp
+++ llvm/lib/Analysis/IVDescriptors.cpp
@@ -1025,6 +1025,18 @@
       // Previous. Nothing left to do.
       if (DT->dominates(Previous, OtherPrev) || Previous == OtherPrev)
         return true;
+
+      // If there are other instructions to be sunk after SinkCandidate, remove
+      // and re-insert SinkCandidate can break those instructions. Bail out for
+      // simplicity.
+      const auto SuccIt = find_if(
+          SinkAfter,
+          [SinkCandidate](const std::pair<Instruction *, Instruction *> &P) {
+            return P.second == SinkCandidate;
+          });
+      if (SuccIt != std::end(SinkAfter))
+        return false;
+
       // Otherwise, Previous comes after OtherPrev and SinkCandidate needs to be
       // re-sunk to Previous, instead of sinking to OtherPrev. Remove
       // SinkCandidate from SinkAfter to ensure it's insert position is updated.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D134083.460889.patch
Type: text/x-patch
Size: 2495 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220916/78b603cd/attachment.bin>