Page MenuHomePhabricator

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

Authored by davide on Oct 26 2016, 3:08 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

davide updated this revision to Diff 75955.Oct 26 2016, 3:08 PM
davide retitled this revision from to [ConstantFold] Get the correct vector type when folding a getelementptr instruction with vector indices.
davide updated this object.
davide added reviewers: majnemer, Bigcheese, RKSimon, spatel.
davide added a subscriber: llvm-commits.
majnemer added inline comments.Oct 26 2016, 3:15 PM
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 updated this revision to Diff 75960.Oct 26 2016, 3:56 PM

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?

davide updated this revision to Diff 75972.Oct 26 2016, 5:51 PM
majnemer added inline comments.Oct 26 2016, 11:06 PM
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 updated this revision to Diff 76083.Oct 27 2016, 12:39 PM

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

majnemer added inline comments.Oct 27 2016, 12:55 PM
lib/IR/ConstantFold.cpp
2063–2069 ↗(On Diff #76083)

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

davide updated this revision to Diff 76096.Oct 27 2016, 2:01 PM

Simplify.

majnemer added inline comments.Oct 27 2016, 2:12 PM
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 updated this revision to Diff 76101.Oct 27 2016, 2:24 PM
davide added inline comments.
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 accepted this revision.Oct 27 2016, 2:44 PM
majnemer edited edge metadata.

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

This revision is now accepted and ready to land.Oct 27 2016, 2:44 PM
davide added inline comments.Oct 27 2016, 4:07 PM
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 added inline comments.Oct 27 2016, 5:03 PM
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.

This revision was automatically updated to reflect the committed changes.