Page MenuHomePhabricator

[InstCombine][SVE] Fix visitGetElementPtrInst for scalable type.
ClosedPublic

Authored by huihuiz on Apr 13 2020, 10:10 PM.

Details

Summary

This patch fix the following issues in InstCombiner::visitGetElementPtrInst

  1. Skip for scalable type if transformation requires fixed size number of vector element.
  2. Skip for scalable type if transformation relies on compile-time known type alloc size.
  3. Use VectorType::getElementCount when scalable property is used to construct new VectorType.
  4. Use TypeSize::getKnownMinSize when minimal size of a scalable type is valid to determine GEP 'inbounds'.
  5. Explicitly call TypeSize::getFixedSize to avoid implicit type conversion to uint64_t.

Diff Detail

Event Timeline

huihuiz created this revision.Apr 13 2020, 10:10 PM

Current upstream crash at llvm/lib/IR/Constants.cpp:1674: static llvm::Constant *llvm::ConstantExpr::getCast(unsigned int, llvm::Constant *, llvm::Type *, bool): Assertion `CastInst::castIsValid(opc, C, Ty) && "Invalid constantexpr cast!"' failed.

take test.ll, run opt -S -instcombine test.ll

define <vscale x 2 x i8*> @gep_index_type_is_scalable(i8* %p) {
  %gep = getelementptr i8, i8* %p, <vscale x 2 x i64> undef
  ret <vscale x 2 x i8*> %gep
}
efriedma added inline comments.Apr 13 2020, 10:46 PM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2494

Can you just use DL.getTypeAllocSize(AI->getAllocatedType()).getKnownMinSize() here, instead of bailing out?

huihuiz updated this revision to Diff 257392.Apr 14 2020, 10:06 AM
huihuiz marked an inline comment as done.
huihuiz edited the summary of this revision. (Show Details)

Thanks Eli for the feedbacks!
Yes, it's valid to use scalable type mininal size for inbounds checking.

This revision is now accepted and ready to land.Apr 14 2020, 11:53 AM
This revision was automatically updated to reflect the committed changes.