[PATCH] D80086: [VPlan] Fix bug45985

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 created this revision.
jaykang10 added reviewers: fhahn, hsaito.
Herald added subscribers: llvm-commits, vkmr, psnobl, rogfer01, rkruppe, bollu, hiraditya.
Herald added a reviewer: rengolin.
Herald added a project: LLVM.
jaykang10 edited the summary of this revision.

https://bugs.llvm.org/show_bug.cgi?id=45958

Let's see simple IR snippet after loop vectorization with VPlanNatviePath.

  vector.body:
    br label %for.body10.preheader67
  
  for.body10.preheader67:                     ; preds =
  %for.cond.cleanup972, %vector.body
    %vec.phi = phi <4 x i64> [ zeroinitializer, %for.cond.cleanup972 ],
  [ %8, %vector.body ]
  
  for.cond.cleanup972:                              ; preds = %for.body1068
    %8 = add nuw nsw <4 x i64> %vec.phi, <i64 1, i64 1, i64 1, i64 1>
    br i1 %10, label %for.cond.cleanup373, label %for.body10.preheader67

As you can see, %vec.phi has wrong incoming basic blocks.

The problem comes from "InnerLoopVectorizer::fixNonInductionPHIs()".
This function has assumption about the order of predecessors as below.

  // The predecessor order is preserved and we can rely on mapping between
  // scalar and vector block predecessors.
  for (unsigned i = 0; i < NumIncomingValues; ++i) {

The original phi's order of predecessors can be different with the new phi's one.


https://reviews.llvm.org/D80086

Files:
  llvm/lib/Transforms/Vectorize/LoopVectorize.cpp


Index: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
===================================================================
--- llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -4048,8 +4048,11 @@
     // getOrCreateVectorValue calls below.
     Builder.SetInsertPoint(NewPhi);
 
-    // The predecessor order is preserved and we can rely on mapping between
-    // scalar and vector block predecessors.
+    // The order of predecessors can be different between OrigPhi BB and NewPhi
+    // BB. Check dominance of incoming values and map incoming value and BB
+    // according to the dominace.
+    SmallVector<Value *, 2> NewIncomingValues;
+    bool NeedSwap = false;
     for (unsigned i = 0; i < NumIncomingValues; ++i) {
       BasicBlock *NewPredBB = VectorBBPredecessors[i];
 
@@ -4060,6 +4063,23 @@
 
       // Scalar incoming value may need a broadcast
       Value *NewIncV = getOrCreateVectorValue(ScIncV, 0);
+
+      // Check dominance of NewIncV with NewPredBB.
+      if (Instruction *NewIncI = dyn_cast<Instruction>(NewIncV)) {
+        if (!DT->dominates(NewIncI, NewPredBB)) {
+          NeedSwap = true;
+        }
+      }
+      NewIncomingValues.push_back(NewIncV);
+    }
+
+    if (NeedSwap) {
+      std::swap(NewIncomingValues[0], NewIncomingValues[1]);
+    }
+
+    for (unsigned i = 0; i < NumIncomingValues; ++i) {
+      BasicBlock *NewPredBB = VectorBBPredecessors[i];
+      Value *NewIncV = NewIncomingValues[i];
       NewPhi->addIncoming(NewIncV, NewPredBB);
     }
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D80086.264500.patch
Type: text/x-patch
Size: 1563 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200517/3480dafc/attachment.bin>