[ConstantFold] Get the correct vector type when folding a getelementptr instruction with vector indices

[davide]

Summary

When we constant fold getelementptr and the indices are vector, the type created is i64 * and not <8 x i64*> which caseuse an assertion to fire in replaceAllUsesWith. I think it's OK just checking for the first index for the reason pointed out in the comment.

[majnemer]

lib/IR/ConstantFold.cpp

2063–2070

Could this be replaced with GetElementPtrInst::getGEPReturnType ?

2065

one or more

test/Transforms/InstCombine/gep-vector.ll

1–19

Can we test the folding more directly? Why do we need the gather call? Couldn't we just have the function ReturnInst contain the GEP ConstantExpr?

[davide]

Fixed typo, simplified test.

lib/IR/ConstantFold.cpp

2063–2070

hmm, I don't see an overload of getGEPReturnType which takes an ArrayRef of Constant but only Value -- do you want me to add one?

[majnemer]

lib/IR/ConstantFold.cpp

2063–2070

I think we can construct an ArrayRef<Value *> from an ArrayRef<Constant *> without violating any safety rules, something like GetElementPtrInst::getGEPReturnType(PtrTy, {Idxs.begin(), Idxs.end()}) or GetElementPtrInst::getGEPReturnType(PtrTy, {Idxs.data(), Idxs.size()})

[davide]

David's comments (also added again the overload which I previously removed because it was unused).

[majnemer]

lib/IR/ConstantFold.cpp

2063–2071

Do we still need this if/else now that we are using GetElementPtrInst::getGEPReturnType?

[davide]

Simplify.

[majnemer]

lib/IR/ConstantFold.cpp

2062

Isn't this a dead store to GEPTy?

2063–2064

This comment is a little confusing because nothing vector related is happening here, we are just calling some function.

[davide]

lib/IR/ConstantFold.cpp

2062

Yes, actually there's more dead code (unless I'm mistaken).

2063–2064

Agree, removed.

[majnemer]

LGTM with the gep instruction replaced with a gep constant expression (all the operands are constant) w/ the test moved to test/Assembler/getelementptr.ll

[davide]

test/Transforms/InstCombine/gep-vector.ll

13–16

I tried to replace this getelemptr with

getelementptr inbounds (i64, i64* undef, <8 x i64> undef)

but I get a parser error. Is this asimmetry between getelementptr constant expression and getelementptr instruction intended, or am I missing something?

[majnemer]

test/Transforms/InstCombine/gep-vector.ll

13–16

Seems to be a bug in LLVM.
For example:

cat t.ll

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

@GV = global i64 0

define <8 x i64*> @patatino2() {
  %el = getelementptr inbounds i64, i64* @GV, <8 x i64> <i64 0, i64 1, i64 0, i64 1, i64 0, i64 1, i64 0, i64 1>
  ret <8 x i64*> %el
}

~/llvm/Debug+Asserts/bin/opt -S t.ll -O2 | ~/llvm/Debug+Asserts/bin/llc -o -
~/llvm/Debug+Asserts/bin/llc: <stdin>:10:18: error: getelementptr index type missmatch
ret <8 x i64*> getelementptr (i64, i64* @GV, <8 x i64> <i64 0, i64 1, i64 0, i64 1, i64 0, i64 1, i64 0, i64 1>)

It passes the verifier but the parser refuses to deal with it. Worth looking into.

lib/IR/ConstantFold.cpp

   Constant *Idx0 = cast<Constant>(Idxs[0]);
   if ((Idxs.size() == 1 && Idx0->isNullValue()))
     return C;
 
   if (isa<UndefValue>(C)) {
     PointerType *PtrTy = cast<PointerType>(C->getType()->getScalarType());
     Type *Ty = GetElementPtrInst::getIndexedType(PointeeTy, Idxs);
     assert(Ty && "Invalid indices for GEP!");
     Type *GEPTy = PointerType::get(Ty, PtrTy->getAddressSpace());
     if (VectorType *VT = dyn_cast<VectorType>(C->getType()))
       GEPTy = VectorType::get(GEPTy, VT->getNumElements());
+    // When the getelementptr has one of more vector indices, it will
+    // return a vector of pointers. We guarantee that all the vectors
+    // in this case will have the same width, so we can just look at
+    // the first one.
+    else if (VectorType *VT = dyn_cast<VectorType>(Idxs[0]->getType()))
+      GEPTy = VectorType::get(GEPTy, VT->getNumElements());
     return UndefValue::get(GEPTy);
   }
 
   if (C->isNullValue()) {
     bool isNull = true;
     for (unsigned i = 0, e = Idxs.size(); i != e; ++i)
       if (!cast<Constant>(Idxs[i])->isNullValue()) {
         isNull = false;
         break;

test/Transforms/InstCombine/gep-vector.ll

+; RUN: opt -instcombine %s -S | FileCheck %s
+
+; CHECK-LABEL: patatino
+; CHECK-NEXT: ret void
+define void @patatino() {
+  %el = getelementptr i64, <8 x i64*> undef, <8 x i64> undef
+  %gather = call <8 x i64> @llvm.masked.gather.v8i64(<8 x i64*> %el, i32 8, <8 x i1> <i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true>, <8 x i64> undef)
+  ret void
+}
+
+; CHECK-LABEL: patatino2
+; CHECK-NEXT: ret void
+define void @patatino2() {
+  %el = getelementptr inbounds i64, i64* undef, <8 x i64> undef
+  %gather = call <8 x i64> @llvm.masked.gather.v8i64(<8 x i64*> %el, i32 8, <8 x i1> <i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true, i1 true>, <8 x i64> undef)
+  ret void
+}
+
+declare <8 x i64> @llvm.masked.gather.v8i64(<8 x i64*>, i32, <8 x i1>, <8 x i64>)