This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by huihuiz on Apr 15 2020, 9:06 PM.

Details

Summary

This patch fix the following issues with visitExtractElementInst:

  1. Restrict VectorUtils::findScalarElement to fixed-length vector. For scalable type, the number of elements in shuffle mask is unknown at compile-time.
  2. Fix out-of-range calculation for fixed-length vector.
  3. Skip scalable type when analysis rely on fixed number of elements.
  4. Add unit tests to check functionality of extractelement for scalable type.

Diff Detail

Event Timeline

huihuiz created this revision.Apr 15 2020, 9:06 PM

Current upstream crash at llvm/include/llvm/ADT/APInt.h:1437: void llvm::APInt::setBit(unsigned int): Assertion `BitPosition < BitWidth && "BitPosition out of range"' failed.

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

define i32 @extractelement_maybe_out_of_range(<vscale x 4 x i32> %a) {
  %r = extractelement <vscale x 4 x i32> %a, i64 4
  ret i32 %r
}

I am also looking at the related issues of shufflevector created by "extractelement, insertelement" pair.
Currently working on InstCombiner::visitInsertElementInst.

efriedma added inline comments.Apr 16 2020, 3:37 PM
llvm/lib/Analysis/VectorUtils.cpp
296

"EltNo < LHSWidth" doesn't make any sense.

llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
339–340

I don't think it makes sense to ignore the "scalable" bit here; it's perfectly sensible to extract the third element from a <vscale x 2 x i64>.

huihuiz updated this revision to Diff 261119.Apr 29 2020, 8:17 PM
huihuiz marked 2 inline comments as done.
huihuiz edited the summary of this revision. (Show Details)

Thanks Eli for the feedback!
Please let me know if the fixes related to shuffle vector make sense to you.

llvm/lib/Analysis/VectorUtils.cpp
296

Update this fix. I think we should check if EltNo is recorded in shuffle mask before calling getMaskValue.

efriedma added inline comments.May 4 2020, 11:41 AM
llvm/lib/Analysis/VectorUtils.cpp
292

I think I'd prefer to avoid calling getMaskValue() and trying to compute LHSWidth for scalable vectors; it's not clear what it should mean.

llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
432

Same as above.

huihuiz updated this revision to Diff 262700.May 7 2020, 11:19 AM
huihuiz marked 2 inline comments as done.
huihuiz edited the summary of this revision. (Show Details)

Address review comments.

This revision is now accepted and ready to land.May 7 2020, 12:42 PM
thakis added a subscriber: thakis.May 7 2020, 1:11 PM

Looks like this breaks check-llvm at least on Linux: http://45.33.8.238/linux/17186/step_12.txt

Can you take a look? If it takes a while to fix, please revert while you investigate.

thakis added a comment.May 7 2020, 1:11 PM

Looks like this breaks check-llvm at least on Linux: http://45.33.8.238/linux/17186/step_12.txt

Can you take a look? If it takes a while to fix, please revert while you investigate.

This revision was automatically updated to reflect the committed changes.

Looks like this breaks check-llvm at least on Linux: http://45.33.8.238/linux/17186/step_12.txt

Can you take a look? If it takes a while to fix, please revert while you investigate.

Already adjusted test check-lines

-; CHECK-NEXT: [[TMP1:%.*]] = insertelement <vscale x 4 x i32> undef, i32 [[VEC_E0]], i32 0
+; CHECK-NEXT: [[TMP1:%.*]] = insertelement <vscale x 4 x i32> [[VEC]], i32 [[VEC_E0]], i32 0

Sorry , it wasn't caught by pre-checkin buildbot.