[SLP]Fix PR43799: Crash on different sizes of GEP indices.

[ABataev]

Summary

If the GEP instructions are going to be vectorized, the indices in those
GEP instructions must be of the same type. Otherwise, the compiler may
crash when trying to build the vector constant.

[spatel]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

4170–4171

I think it's already a fuzzer corner case that we have different types in the bug report example, but let's take that 1 step further: what if the index type in IR is larger than getIntPtrType()? We might illegally truncate a large constant value.

Safer to just give up completely if we find mismatched GEP index types?

[ABataev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

4170–4171

Maybe, just give up if GEP at least one index type is larger than intptr?

[spatel]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

4170–4171

That should work, but it seems like we are adding code complexity for no real-world reason. Unless I'm misunderstanding - do we have real code / tests where the GEP indexes are different sizes?

[ABataev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

4170–4171

It does not add a lot of complexity here, just a few additional lines of code.

[xbolva00]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

4170–4171

@spatel:
Recent memcmp related patch uses CreateGEP_64 (dont know exact name atm; index is i64) - could it be affected with your proposed stronger bail out?. We should be careful to not pessimize the code generated by own passes.

[ABataev]

Added a check for GEP indexes greater than pointer size.

[lebedev.ri]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

4170–4171

https://llvm.org/docs/LangRef.html#langref-datalayout

Data Layout
p[n]:<size>:<abi>:<pref>:<idx>
This specifies the size of a pointer and its <abi> and <pref>erred alignments for address space n. The fourth parameter <idx> is a size of index that used for address calculation. If not specified, the default index size is equal to the pointer size. All sizes are in bits. The address space, n, is optional, and if not specified, denotes the default address space 0. The value of n must be in the range [1,2^23).

[spatel]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

4170–4171

For reference, I think the patch that @xbolva00 mentioned is:
D69507

(...unfortunately, expandmemcmp is still a codegen pass - D60318 / rL371507 - so that particular case can't be in play yet for the question/concern raised in this patch)

Given the LangRef specification, I'm surprised that we're encouraging hard-coding the index size via the Builder API rather than deriving it from the DataLayout.

So it seems like we've made a mess, and now we want to deal with that mess here. I still don't see any real-world reason for this patch to try so hard, but if we insist on it, then we should be using:

DL->getIndexTypeSizeInBits()

?

[ABataev]

Address comments.

[spatel]

LGTM

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

case Instruction::GetElementPtr: {
           BS.cancelScheduling(VL, VL0);
           newTreeEntry(VL, None /*not vectorized*/, S, UserTreeIdx,
                        ReuseShuffleIndicies);
           return;
         }
       }
 
       // We don't combine GEPs with non-constant indexes.
+      Type *Ty1 = VL0->getOperand(1)->getType();
       for (Value *V : VL) {
         auto Op = cast<Instruction>(V)->getOperand(1);
-        if (!isa<ConstantInt>(Op)) {
+        if (!isa<ConstantInt>(Op) ||
+            (Op->getType() != Ty1 &&
+             Op->getType()->getScalarSizeInBits() >
+                 DL->getIndexSizeInBits(
+                     V->getType()->getPointerAddressSpace()))) {
           LLVM_DEBUG(dbgs()
                      << "SLP: not-vectorizable GEP (non-constant indexes).\n");
           BS.cancelScheduling(VL, VL0);
           newTreeEntry(VL, None /*not vectorized*/, S, UserTreeIdx,
                        ReuseShuffleIndicies);
           return;
         }
       }
switch (ShuffleOrOp) {
     case Instruction::GetElementPtr: {
       setInsertPointAfterBundle(E);
 
       Value *Op0 = vectorizeTree(E->getOperand(0));
 
       std::vector<Value *> OpVecs;
       for (int j = 1, e = cast<GetElementPtrInst>(VL0)->getNumOperands(); j < e;
            ++j) {
-        Value *OpVec = vectorizeTree(E->getOperand(j));
+        ValueList &VL = E->getOperand(j);
+        // Need to cast all elements to the same type before vectorization to
+        // avoid crash.
+        Type *VL0Ty = VL0->getOperand(j)->getType();
+        Type *Ty = llvm::all_of(
+                       VL, [VL0Ty](Value *V) { return VL0Ty == V->getType(); })
+                       ? VL0Ty
+                       : DL->getIndexType(cast<GetElementPtrInst>(VL0)
+                                              ->getPointerOperandType()
+                                              ->getScalarType());
+        for (Value *&V : VL) {
+          auto *CI = cast<ConstantInt>(V);
+          V = ConstantExpr::getIntegerCast(CI, Ty,
+                                           CI->getValue().isSignBitSet());
+        }
+        Value *OpVec = vectorizeTree(VL);
         OpVecs.push_back(OpVec);
       }
 
       Value *V = Builder.CreateGEP(
           cast<GetElementPtrInst>(VL0)->getSourceElementType(), Op0, OpVecs);
       if (Instruction *I = dyn_cast<Instruction>(V))
         V = propagateMetadata(I, E->Scalars);
 

llvm/test/Transforms/SLPVectorizer/X86/crash_gep.ll

entry:
   %add.ptr = getelementptr inbounds i64, i64* %0, i64 1
   %1 = ptrtoint i64* %add.ptr to i64
   %arrayidx = getelementptr inbounds i64, i64* %0, i64 2
   store i64 %1, i64* %arrayidx, align 8
   %2 = ptrtoint i64* %arrayidx to i64
   store i64 %2, i64* %add.ptr, align 8
   ret i32 undef
 }
+
+define void @PR43799() {
+; CHECK-LABEL: @PR43799(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[BODY:%.*]]
+; CHECK:       body:
+; CHECK-NEXT:    br label [[BODY]]
+; CHECK:       epilog:
+; CHECK-NEXT:    ret void
+;
+entry:
+  br label %body
+
+body:
+  %p.1.i19 = phi i8* [ undef, %entry ], [ %incdec.ptr.i.7, %body ]
+  %lsr.iv17 = phi i8* [ undef, %entry ], [ %scevgep113.7, %body ]
+  %incdec.ptr.i.7 = getelementptr inbounds i8, i8* undef, i32 1
+  %scevgep113.7 = getelementptr i8, i8* undef, i64 1
+  br label %body
+
+epilog:
+  ret void
+}