[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

2–20

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

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

12–15

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

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.

include/llvm/IR/Instructions.h

 
   /// Returns the address space of the pointer operand.
   unsigned getPointerAddressSpace() const {
     return getPointerOperandType()->getPointerAddressSpace();
   }
 
   /// Returns the pointer type returned by the GEP
   /// instruction, which may be a vector of pointers.
+  static Type *getGEPReturnType(Value *Ptr, ArrayRef<Value *> IdxList) {
+    return getGEPReturnType(
+      cast<PointerType>(Ptr->getType()->getScalarType())->getElementType(),
+      Ptr, IdxList);
+  }
   static Type *getGEPReturnType(Type *ElTy, Value *Ptr,
                                 ArrayRef<Value *> IdxList) {
     Type *PtrTy = PointerType::get(checkGEPType(getIndexedType(ElTy, IdxList)),
                                    Ptr->getType()->getPointerAddressSpace());
     // Vector GEP
     if (Ptr->getType()->isVectorTy()) {
       unsigned NumElem = Ptr->getType()->getVectorNumElements();
       return VectorType::get(PtrTy, NumElem);

lib/IR/ConstantFold.cpp

                                                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;
         break;
       }
     if (isNull) {
       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());

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
+}