[PATCH] D103153: [InstCombine] Add fold for extracting known elements from a stepvector

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:357
+                cast<VectorType>(EI.getVectorOperandType())->getScalarType(),
+                IndexC->getZExtValue());
+            return replaceInstUsesWith(EI, Idx);
----------------
CarolineConcatto wrote:
> foad wrote:
> > CarolineConcatto wrote:
> > > foad wrote:
> > > > Just use getValue here.
> > > It crashes when I replace 
> > > IndexC->getZExtValue() by IndexC->getValue()
> > > 
> > > ```
> > > Assertion `C->getType() == Ty->getScalarType() && "ConstantInt type doesn't match the type implied by its value!"' failed.
> > > ```
> > OK, it's a bit more complicated if the index type does not match the vector element type. You also need to give up if the index value is too large to fit in the vector element type (this is the case where the langref says "If the sequence value exceeds the allowed limit for the element type then the result for that lane is undefined").
> > 
> > How about something like:
> > ```
> > Type *Ty = EI.getType();
> > unsigned BitWidth = Ty->getIntegerBitWidth();
> > if (IndexC->getValue().getActiveBits() > BitWidth)
> >   break; // with a suitable comment
> > auto *Idx = ConstantInt::get(Ty, IndexC->getValue().zextOrTrunc(BitWidth));
> > ```
> > You should also add at least one test where the extractelement index type is wider than the stepvector element type.
> Hi @foad,
> I have tried a combination of your suggestion, because as is it did not worked properly.
> 1)Replacing the tests and updating the Idx to use getValue
> When I replace:
> 
> ```
>   if (IndexC->getValue().ult(NumElts)). //test 1
> ```
> by
> 
> ```
> if (IndexC->getValue().getActiveBits() > BitWidth) //test2
> ```
> 
> The invalid test (ext_lane_invalid_from_stepvec)  and out of range( ext_lane_out_of_range_from_stepvec) return some values, garbage and 5.
> 
> 2)Having the original test, but updating the Idx to use getValue
> And when  I leave the first test(test 1), the second test (test 2) is not needed. 
> 
> I added the test you suggested too when the index is wider than the vector element and works.
> 
> Let me know if that is ok and is what you had in mind.
You can see the problem with a test case like this:
```
define i8 @ext_lane300_from_stepvec() {
; CHECK-LABEL: @ext_lane300_from_stepvec(
; CHECK-NEXT:  entry:
; CHECK-NEXT:    ret i8 44
;
entry:
  %0 = call <vscale x 512 x i8> @llvm.experimental.stepvector.nxv512i8()
  %1 = extractelement <vscale x 512 x i8> %0, i64 300
  ret i8 %1
}
```
Currently you optimize the result to 44, which is wrong. You should either refuse to optimize, or optimize it to undef ("If the sequence value exceeds the allowed limit for the element type then the result for that lane is undefined").

I was trying to suggest something like:
```
if (IndexC->getValue().ult(NumElts)) {
  if (IndexC->getValue().getActiveBits() > BitWidth) {
    // IndexC can't be safely truncated to BitWidth bits
    Idx = UndefValue::get(Ty); // or just give up?
  } else {
    Idx = ConstantInt::get(Ty, IndexC->getValue().zextOrTrunc(BitWidth));
  }
}
```


================
Comment at: llvm/test/Transforms/InstCombine/vscale_extractelement.ll:250-251
+
+declare <vscale x 4 x i64> @llvm.experimental.stepvector.nxv4i64();
+declare <vscale x 4 x i32> @llvm.experimental.stepvector.nxv4i32();
----------------
Don't need `;` after a declaration. In LLVM IR `;` starts a comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103153/new/

https://reviews.llvm.org/D103153