[SLP]Fix insertpoint of the extractellements instructions to avoid reshuffle crash.

[ABataev]

Summary

Need to set the insertpoint for extractelement to point to the first
instruction in the node to avoid possible crash during external uses
combine process. Without it we may endup with the incorrect
transformation.

[RKSimon]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

7696

At what point when extending a capture list should we just use [&]? @dblaikie was commenting on this not so long ago.

[ABataev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

7696

I don't see any rules related to the list of captures in LLVM coding standard https://llvm.org/docs/CodingStandards.html

[ABataev]

Ping!

[RKSimon]

LGTM

llvm/test/Transforms/SLPVectorizer/X86/extractelement-insertpoint.ll

30

maybe cleanup these variable names?

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

for (Value *V : E->Scalars) {
              "Different nodes should have different DFS numbers");
       if (NodeA->getDFSNumIn() < NodeB->getDFSNumIn())
         LastInst = I;
     }
     BB = LastInst->getParent();
     return LastInst;
   };
 
-  auto &&FindFirstInst = [E, Front]() {
+  auto &&FindFirstInst = [E, Front, this]() {
     Instruction *FirstInst = Front;
     for (Value *V : E->Scalars) {
       auto *I = dyn_cast<Instruction>(V);
       if (!I)
         continue;
+      if (FirstInst->getParent() == I->getParent()) {
         if (I->comesBefore(FirstInst))
           FirstInst = I;
+        continue;
+      }
+      assert(isVectorLikeInstWithConstOps(FirstInst) &&
+             isVectorLikeInstWithConstOps(I) &&
+             "Expected vector-like insts only.");
+      if (!DT->isReachableFromEntry(FirstInst->getParent())) {
+        FirstInst = I;
+        continue;
+      }
+      if (!DT->isReachableFromEntry(I->getParent()))
+        continue;
+      auto *NodeA = DT->getNode(FirstInst->getParent());
+      auto *NodeB = DT->getNode(I->getParent());
+      assert(NodeA && "Should only process reachable instructions");
+      assert(NodeB && "Should only process reachable instructions");
+      assert((NodeA == NodeB) ==
+                 (NodeA->getDFSNumIn() == NodeB->getDFSNumIn()) &&
+             "Different nodes should have different DFS numbers");
+      if (NodeA->getDFSNumIn() > NodeB->getDFSNumIn())
+        FirstInst = I;
     }
     return FirstInst;
   };
 
   // Set the insert point to the beginning of the basic block if the entry
   // should not be scheduled.
   if (E->State != TreeEntry::NeedToGather &&
-      doesNotNeedToSchedule(E->Scalars)) {
+      (doesNotNeedToSchedule(E->Scalars) ||
+       all_of(E->Scalars, isVectorLikeInstWithConstOps))) {
     Instruction *InsertInst;
-    if (all_of(E->Scalars, isUsedOutsideBlock))
+    if (all_of(E->Scalars, [](Value *V) {
+          return !isVectorLikeInstWithConstOps(V) && isUsedOutsideBlock(V);
+        }))
       InsertInst = FindLastInst();
     else
       InsertInst = FindFirstInst();
     return *InsertInst;
   }
 
   // The last instruction in the bundle in program order.
   Instruction *LastInst = nullptr;
case Instruction::PHI: {
 
       assert(NewPhi->getNumIncomingValues() == PH->getNumIncomingValues() &&
              "Invalid number of incoming values");
       return V;
     }
 
     case Instruction::ExtractElement: {
       Value *V = E->getSingleOperand(0);
-      Builder.SetInsertPoint(VL0);
+      setInsertPointAfterBundle(E);
       ShuffleBuilder.addInversedMask(E->ReorderIndices);
       ShuffleBuilder.addMask(E->ReuseShuffleIndices);
       V = ShuffleBuilder.finalize(V);
       E->VectorizedValue = V;
       return V;
     }
     case Instruction::ExtractValue: {
       auto *LI = cast<LoadInst>(E->getSingleOperand(0));

llvm/test/Transforms/SLPVectorizer/X86/extractelement-insertpoint.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -slp-vectorizer -mtriple=x86_64-grtev4-linux-gnu -o - < %s | FileCheck %s
+
+define i32 @crash() {
+; CHECK-LABEL: @crash(
+; CHECK-NEXT:  label:
+; CHECK-NEXT:    [[ADD:%.*]] = fadd <2 x double> zeroinitializer, zeroinitializer
+; CHECK-NEXT:    [[SHUFFLE:%.*]] = shufflevector <2 x double> [[ADD]], <2 x double> poison, <2 x i32> <i32 1, i32 0>
+; CHECK-NEXT:    [[TMP0:%.*]] = shufflevector <2 x double> zeroinitializer, <2 x double> [[SHUFFLE]], <2 x i32> <i32 2, i32 1>
+; CHECK-NEXT:    [[TMP1:%.*]] = fmul <2 x double> [[SHUFFLE]], zeroinitializer
+; CHECK-NEXT:    [[TMP2:%.*]] = extractelement <2 x double> [[TMP1]], i32 0
+; CHECK-NEXT:    [[TMP3:%.*]] = extractelement <2 x double> [[TMP1]], i32 1
+; CHECK-NEXT:    [[ADD1:%.*]] = fadd double [[TMP2]], [[TMP3]]
+; CHECK-NEXT:    [[MUL1:%.*]] = fmul double [[ADD1]], 0.000000e+00
+; CHECK-NEXT:    store double [[MUL1]], double* null, align 16
+; CHECK-NEXT:    ret i32 0
+;
+label:
+  %0 = extractelement <2 x double> zeroinitializer, i64 1
+  %1 = extractelement <2 x double> zeroinitializer, i64 0
+  %add = fadd <2 x double> zeroinitializer, zeroinitializer
+  %extract1 = extractelement <2 x double> %add, i64 1
+  %2 = fmul double %extract1, %1
+  %insert = insertelement <2 x double> zeroinitializer, double %extract1, i64 0
+  %extract0 = extractelement <2 x double> %add, i64 0
+  %mul = fmul double %extract0, %0
+  %add1 = fadd double %2, %mul
+  %mul1 = fmul double %add1, 0.000000e+00
+  store double %mul1, double* null, align 16
+  ret i32 0
+}