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

[Caroline via Phabricator via llvm-commits]

CarolineConcatto marked 2 inline comments as done.
CarolineConcatto added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:357
+                cast<VectorType>(EI.getVectorOperandType())->getScalarType(),
+                IndexC->getZExtValue());
+            return replaceInstUsesWith(EI, Idx);
----------------
foad wrote:
> 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));
>   }
> }
> ```
Hi @foad,
Thank you for pointing that problem again to me.
I did miss that before, sorry.
I added new tests to cover this now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103153