[PATCH] D64285: [InstCombine] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y))

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Sorry for so may triparounds :(
Please re-re-precommit the tests first.
I guess this will work, with 2 nits addressed :)

In D64285#1580958 <https://reviews.llvm.org/D64285#1580958>, @spatel wrote:

> In D64285#1580944 <https://reviews.llvm.org/D64285#1580944>, @nikic wrote:
>
> > So to answer your question, it's about both code duplication and redundant matching. And while the death by a thousand cuts may be unavoidable, we should still try to not hasten it along unduly...
>
>
> I agree. Let's not add extra code for the 'exact' case if we don't need to. Ie, if we can do all of these transforms in instcombine alone by dropping the 'exact' flag, do that. Put another way: unless there's evidence that dropping the exact flag is going to cause a missed optimization, go with the 'less code is better' solution. We have precedence in instcombine/instsimplify for that trade-off.


I think you were thinking of something else here, as this does not represent the problem at hand.
We fully can do this fold within instcombine, preserving `exact` when possible.
By having instsimplify fold we, well, enrich instsimplify utility.



================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:565-567
+    bool IsExact = (!cast<Instruction>(TrueVal)->isExact() && Ashr->isExact())
+                       ? false
+                       : Ashr->isExact();
----------------
This is way more covoluted than it should be:
```
bool IsExact = Ashr->isExact() && cast<Instruction>(TrueVal)->isExact();
```


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:568
+                       : Ashr->isExact();
+    return Builder.CreateAShr(Ashr->getOperand(0), Ashr->getOperand(1),
+                              Ashr->getName(), IsExact);
----------------
You already matched `X` and `Y`.



================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:569
+    return Builder.CreateAShr(Ashr->getOperand(0), Ashr->getOperand(1),
+                              Ashr->getName(), IsExact);
+  }
----------------
[optional] Do we really care about the name? I'd think we should take one from `select` then, not that it is accessible here.


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

https://reviews.llvm.org/D64285