[PATCH] D154791: [InstCombine] Transform bitwise (A >> C - 1, zext(icmp)) -> zext (bitwise(A < 0, icmp)) fold.

[Hongyu Chen via Phabricator via llvm-commits]

XChy added a comment.

In D154791#4483474 <https://reviews.llvm.org/D154791#4483474>, @nikic wrote:

> The basic idea here is reasonable, but you need to be very careful about infinite loops: If you replace the shift with zext+icmp and it does *not* get folded afterwards, it will be converted back to the shift, and so on. I don't think the fold is guaranteed to happen, e.g. due to some unlucky interaction with shouldOptimizeCast().
>
> I would recommend to instead directly produce the zext(binop(icmp, icmp)) sequence, rather than letting the following fold handle it.
>
> Please add:
>
> - Multi-use test.
> - Test where we do not get any beneficial fold out of converting the lshr back into an icmp.
>
> Depending on how the latter case looks like, we might want to further limit this -- e.g. does it make sense to do this if the lshr and icmp work on different variables or not?

Thanks for your review! I agree with you. Actually, I came across infinite loops during developing and seem to solve it by letting the following fold handle IR. But it's just solved with few tests.

My original purpose is to use **foldAndOrOfICmps** to fold icmps as  fold `and/or( A < 0, icmp)`. From my perspective, if **foldAndOrOfICmps** doesn't fold the transformed IR, this fold should not happend.

I will add related tests here and try to find out where we do not get any beneficial fold later.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1718
+  //   -> and/or(zext(A < 0), zext(icmp))
+  //   -> zext(and/or(A < 0, icmp))
+  auto MatchBitwiseICmpZeroWithICmp = [&](Value *&Op0, Value *Op1) {
----------------
goldstein.w.n wrote:
> 'and/or' -> 'bitwise' no?
Maybe `and/or` is right, since **foldAndOrOfICmps** do not fold `xor`.
I'm not sure what `xor(icmp,icmp)` is folded into.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1723
+    bool IsMatched =
+        match(Op0, m_LShr(m_Value(A),
+                          m_SpecificInt(Op0->getType()->getScalarSizeInBits() - 1))) &&
----------------
goldstein.w.n wrote:
> `lshr`? The comments are all `shl`. Can you clarify one of them (looks like comments/summary is wrong).
You're right. I mess it up in last patch too. I'll fix it.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1726
+        match(Op1, m_ZExt(m_ICmp(Pred, m_Value(), m_Value())));
+    if (IsMatched) {
+      Op0 = Builder.CreateZExt(
----------------
goldstein.w.n wrote:
> Is there a reason you create `IsMatched` as opposed to just embedding the `match(...)` logic in the if statement?
No, it's my personal habit to set a boolean variable when expression is too long. If needed, I can remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154791