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

[Caroline via Phabricator via llvm-commits]

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:
> > > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103153