[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 ↗

(On Diff #75955)

Could this be replaced with GetElementPtrInst::getGEPReturnType ?

2065 ↗

(On Diff #75955)

one or more

test/Transforms/InstCombine/gep-vector.ll

1–19 ↗

(On Diff #75955)

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 ↗

(On Diff #75955)

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 ↗

(On Diff #75955)

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–2069 ↗

(On Diff #76083)

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

[davide]

Simplify.

[majnemer]

lib/IR/ConstantFold.cpp

2062 ↗

(On Diff #76096)

Isn't this a dead store to GEPTy?

2063–2064 ↗

(On Diff #76096)

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 ↗

(On Diff #76096)

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

2063–2064 ↗

(On Diff #76096)

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

12–15 ↗

(On Diff #76101)

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

12–15 ↗

(On Diff #76101)

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.

llvm/trunk/lib/IR/ConstantFold.cpp

static Constant *ConstantFoldGetElementPtrImpl(Type *PointeeTy, Constant *C,
                                                bool inBounds,
                                                ArrayRef<IndexTy> Idxs) {
   if (Idxs.empty()) return C;
   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());
+    Type *GEPTy = GetElementPtrInst::getGEPReturnType(C,
+        makeArrayRef((Value * const *)Idxs.data(), Idxs.size()));
     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;

llvm/trunk/test/Transforms/InstCombine/gep-vector.ll

+; RUN: opt -instcombine %s -S | FileCheck %s
+
+; CHECK-LABEL: patatino
+; CHECK-NEXT: ret <8 x i64*> undef
+define <8 x i64*> @patatino() {
+  %el = getelementptr i64, <8 x i64*> undef, <8 x i64> undef
+  ret <8 x i64*> %el
+}
+
+; CHECK-LABEL: patatino2
+; CHECK-NEXT: ret <8 x i64*> undef
+define <8 x i64*> @patatino2() {
+  %el = getelementptr inbounds i64, i64* undef, <8 x i64> undef
+  ret <8 x i64*> %el
+}