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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
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) {
----------------
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.


================
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(
----------------
XChy wrote:
> 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.
Its fine.


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

https://reviews.llvm.org/D154791