Page MenuHomePhabricator

[InstSimplify][SVE] Fix SimplifyGEPInst for scalable vector.
ClosedPublic

Authored by huihuiz on Mar 9 2020, 10:18 PM.

Details

Summary

Skip folds that rely on DataLayout::getTypeAllocSize(). For scalable
vector, only minimal type alloc size is known at compile-time.

Diff Detail

Event Timeline

huihuiz created this revision.Mar 9 2020, 10:18 PM

take test.ll, run: opt -S -instsimplify test.ll -o -

define <vscale x 4 x <vscale x 4 x i32>*> @getelementptr_2() {
  %ptr = getelementptr <vscale x 4 x i32>, <vscale x 4 x i32>* null, <vscale x 4 x i64> undef
  ret <vscale x 4 x <vscale x 4 x i32>*> %ptr
}

current upstream crash at: llvm/include/llvm/Support/TypeSize.h:126: uint64_t llvm::TypeSize::getFixedSize() const: Assertion `!IsScalable && "Request for a fixed size on a scalable object"' failed.

This patch depends on D74386, otherwise we get another crash at: llvm/lib/IR/Value.cpp:408: void llvm::Value::doRAUW(llvm::Value *, llvm::Value::ReplaceMetadataUses): Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!"' failed.

efriedma added inline comments.Mar 11 2020, 1:24 PM
llvm/test/Transforms/InstSimplify/vscale.ll
14

@getelementptr_1 passes without this patch, right?

22

Please add testcases with a vscale'ed source type that doesn't simplify to a constant.

Please add a testcase like getelementptr <vscale x 4 x i32>, <vscale x 4 x i32>* null, i64 %x (where the return type is a pointer to a vector, rather than a vector of pointers to vectors).

huihuiz updated this revision to Diff 250039.Mar 12 2020, 1:34 PM
huihuiz marked 2 inline comments as done.

Add more test to check the code path, as suggested.

llvm/test/Transforms/InstSimplify/vscale.ll
14

Yes, @getelementptr_1 passes without this patch.

efriedma accepted this revision.Mar 12 2020, 3:28 PM

LGTM

llvm/lib/Analysis/InstructionSimplify.cpp
4085

We should probably consider adding an API to Type to check whether the size of a type is a compile-time constant, so we don't repeat SrcTy->isVectorTy() ? SrcTy->getVectorIsScalable() : false all over the place. I guess that doesn't have to happen here, though.

This revision is now accepted and ready to land.Mar 12 2020, 3:28 PM
huihuiz marked an inline comment as done.Mar 16 2020, 11:44 AM
huihuiz added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
4085

Thanks Eli for the feedback! I will post a patch for this.

This revision was automatically updated to reflect the committed changes.