[SLP]Fix a crash in gathered loads analysis.

[ABataev]

Summary

Need to check that the minimum acceptable vector factor is at least 2,
not 0, to avoid compiler crash during gathered loads analysis.

[anton-afanasyev]

LGTM

[spatel]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

7454

Similar logic...

7565

And again...can we consolidate these into a helper call to avoid the problem consistently?

[sdesmalen]

Thanks for fixing!

llvm/test/Transforms/SLPVectorizer/AArch64/loads-gather-crash.ll

4 ↗

(On Diff #362724)

Could you name the test to something more descriptive, e.g. gather-load-min-required-vf-2.ll (feel free to come up with something better) and also add a comment explaining why the code should not be slp-vectorized?

[ABataev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

7454

Will try.

llvm/test/Transforms/SLPVectorizer/AArch64/loads-gather-crash.ll

4 ↗

(On Diff #362724)

Ok, no problem.

[ABataev]

Address comments.

[spatel]

Code changes LGTM. No test changes in the updated revision though?
Note that there's a new bugzilla report that has another potential test example for this bug:
https://llvm.org/PR51275

[ABataev]

In D107058#2915078, @spatel wrote:

Code changes LGTM. No test changes in the updated revision though?
Note that there's a new bugzilla report that has another potential test example for this bug:
https://llvm.org/PR51275

No test changes and actually there should not be any. It is a corner case.

[spatel]

In D107058#2915119, @ABataev wrote:

In D107058#2915078, @spatel wrote:

Code changes LGTM. No test changes in the updated revision though?
Note that there's a new bugzilla report that has another potential test example for this bug:
https://llvm.org/PR51275

No test changes and actually there should not be any. It is a corner case.

Ok - I saw the earlier request about adding a comment, and the file name changed, so maybe that’s enough. I didn’t try to reduce the example from bugzilla, but that shows it is possible to induce from legitimate C source code.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

unsigned getMaxVecRegSize() const {
     return MaxVecRegSize;
   }
 
   // \returns minimum vector register size as set by cl::opt.
   unsigned getMinVecRegSize() const {
     return MinVecRegSize;
   }
 
+  unsigned getMinVF(unsigned Sz) const {
+    return std::max(2U, getMinVecRegSize() / Sz);
+  }
+
   unsigned getMaximumVF(unsigned ElemWidth, unsigned Opcode) const {
     unsigned MaxVF = MaxVFOption.getNumOccurrences() ?
       MaxVFOption : TTI->getMaximumVF(ElemWidth, Opcode);
     return MaxVF ? MaxVF : UINT_MAX;
   }
 
   /// Check if homogeneous aggregate is isomorphic to some VectorType.
   /// Accepts homogeneous multidimensional aggregate of scalars/vectors like
if (E->State == TreeEntry::NeedToGather) {
     if (VL.size() > 2 && E->getOpcode() == Instruction::Load &&
         !E->isAltShuffle()) {
       BoUpSLP::ValueSet VectorizedLoads;
       unsigned StartIdx = 0;
       unsigned VF = VL.size() / 2;
       unsigned VectorizedCnt = 0;
       unsigned ScatterVectorizeCnt = 0;
       const unsigned Sz = DL->getTypeSizeInBits(E->getMainOp()->getType());
-      for (unsigned MinVF = getMinVecRegSize() / (2 * Sz); VF >= MinVF;
-           VF /= 2) {
+      for (unsigned MinVF = getMinVF(2 * Sz); VF >= MinVF; VF /= 2) {
         for (unsigned Cnt = StartIdx, End = VL.size(); Cnt + VF <= End;
              Cnt += VF) {
           ArrayRef<Value *> Slice = VL.slice(Cnt, VF);
           if (!VectorizedLoads.count(Slice.front()) &&
               !VectorizedLoads.count(Slice.back()) && allSameBlock(Slice)) {
             SmallVector<Value *> PointerOps;
             OrdersType CurrentOrder;
             LoadsState LS = canVectorizeLoads(Slice, Slice.front(), *TTI, *DL,
while (I != E && !VectorizedStores.count(Stores[I])) {
       I = ConsecutiveChain[I].first;
     }
     assert(!Operands.empty() && "Expected non-empty list of stores.");
 
     unsigned MaxVecRegSize = R.getMaxVecRegSize();
     unsigned EltSize = R.getVectorElementSize(Operands[0]);
     unsigned MaxElts = llvm::PowerOf2Floor(MaxVecRegSize / EltSize);
 
-    unsigned MinVF = std::max(2U, R.getMinVecRegSize() / EltSize);
+    unsigned MinVF = R.getMinVF(EltSize);
     unsigned MaxVF = std::min(R.getMaximumVF(EltSize, Instruction::Store),
                               MaxElts);
 
     // FIXME: Is division-by-2 the correct step? Should we assert that the
     // register size is a power-of-2?
     unsigned StartIdx = 0;
     for (unsigned Size = MaxVF; Size >= MinVF; Size /= 2) {
       for (unsigned Cnt = StartIdx, E = Operands.size(); Cnt + Size <= E;) {
if (!isa<InsertElementInst>(V) && !isValidElementType(Ty)) {
                << "Cannot SLP vectorize list: type "
                << rso.str() + " is unsupported by vectorizer";
       });
       return false;
     }
   }
 
   unsigned Sz = R.getVectorElementSize(I0);
-  unsigned MinVF = std::max(2U, R.getMinVecRegSize() / Sz);
+  unsigned MinVF = R.getMinVF(Sz);
   unsigned MaxVF = std::max<unsigned>(PowerOf2Floor(VL.size()), MinVF);
   MaxVF = std::min(R.getMaximumVF(Sz, S.getOpcode()), MaxVF);
   if (MaxVF < 2) {
     R.getORE()->emit([&]() {
       return OptimizationRemarkMissed(SV_NAME, "SmallVF", I0)
              << "Cannot SLP vectorize list: vectorization factor "
              << "less than 2 is not supported";
     });

llvm/test/Transforms/SLPVectorizer/AArch64/gather-load-min-required-vf-2.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -slp-vectorizer -mtriple=aarch64-unknown-linux-gnu < %s | FileCheck %s
+
+define void @foo() local_unnamed_addr {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = load volatile double, double* poison, align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = load volatile double, double* poison, align 8
+; CHECK-NEXT:    [[TMP2:%.*]] = load volatile double, double* poison, align 8
+; CHECK-NEXT:    [[TMP3:%.*]] = load volatile double, double* poison, align 8
+; CHECK-NEXT:    br label [[FOR_BODY:%.*]]
+; CHECK:       for.body:
+; CHECK-NEXT:    [[D30_0734:%.*]] = phi double [ undef, [[FOR_BODY]] ], [ [[TMP0]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    [[D01_0733:%.*]] = phi double [ undef, [[FOR_BODY]] ], [ [[TMP1]], [[ENTRY]] ]
+; CHECK-NEXT:    [[D11_0732:%.*]] = phi double [ undef, [[FOR_BODY]] ], [ [[TMP2]], [[ENTRY]] ]
+; CHECK-NEXT:    [[D21_0731:%.*]] = phi double [ undef, [[FOR_BODY]] ], [ [[TMP3]], [[ENTRY]] ]
+; CHECK-NEXT:    br label [[FOR_BODY]]
+;
+entry:
+  %0 = load volatile double, double* poison, align 8
+  %1 = load volatile double, double* poison, align 8
+  %2 = load volatile double, double* poison, align 8
+  %3 = load volatile double, double* poison, align 8
+  br label %for.body
+
+for.body:                                         ; preds = %for.body, %entry
+  %d30.0734 = phi double [ undef, %for.body ], [ %0, %entry ]
+  %d01.0733 = phi double [ undef, %for.body ], [ %1, %entry ]
+  %d11.0732 = phi double [ undef, %for.body ], [ %2, %entry ]
+  %d21.0731 = phi double [ undef, %for.body ], [ %3, %entry ]
+  br label %for.body
+}