[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 inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1726
+                          m_SpecificInt(Op0->getType()->getScalarSizeInBits() - 1))) &&
+        match(Op1, m_ZExt(m_ICmp(Pred, m_Value(), m_Value()))) &&
+        LogicOpc != Instruction::Xor;
----------------
goldstein.w.n wrote:
> Does `Op1` need to be an `icmp`, or can it just be any `i1`?
When folding `A >> BW - 1  -> A < 0`, there are many possible folds for `(A < 0) bitwise (icmp)`.
However, if replacing `icmp` with an arbitrary i1, it seldom folds and just produces this single fold `A >> BW - 1  -> A < 0`, which is inefficient.


================
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:
> XChy wrote:
> > 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.
> In that case, you need to only match `and/or` and probably should mention that in the summary.
I noticed **foldXorOfICmps** fold just now. Maybe I can add some related tests here.


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

https://reviews.llvm.org/D154791