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

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added a comment.

Thank you @foad for your fast review. I've tried to address all of them.
I believe one of the changes you've asked I could not apply.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:351
+    if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(SrcVec)) {
+      if (Intrinsic::ID IID = II->getIntrinsicID()) {
+        switch (IID) {
----------------
foad wrote:
> You don't need this "if".
Ok, Indeed I don't need it.
I was adding because I thought it could make it less prone to crash in case it was not an intrinsic call, as I see similar tests being done in other parts of the code.
But I agree that because the first if should be enough


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:354
+        case Intrinsic::experimental_stepvector:
+          if (IndexC->getZExtValue() <= NumElts) {
+            auto *Idx = ConstantInt::get(
----------------
foad wrote:
> Should be "<" not "<=", surely?
Hi @foad,
Yes, you are correct! I left it there by mistake. 
Thank you for pointing it out.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:354
+        case Intrinsic::experimental_stepvector:
+          if (IndexC->getZExtValue() <= NumElts) {
+            auto *Idx = ConstantInt::get(
----------------
foad wrote:
> CarolineConcatto wrote:
> > foad wrote:
> > > Should be "<" not "<=", surely?
> > Hi @foad,
> > Yes, you are correct! I left it there by mistake. 
> > Thank you for pointing it out.
> > 
> `IndexC->getValue().ult(NumElts)` seems safer, since getZExtValue can truncate wide values.
Thank you @foad!
I did not know I could use that.
I believe I can use this in the other patch that I need to do the same test.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:357
+                cast<VectorType>(EI.getVectorOperandType())->getScalarType(),
+                IndexC->getZExtValue());
+            return replaceInstUsesWith(EI, Idx);
----------------
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.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103153