[PATCH] D147597: [InstCombine] icmp(X | LHS, RHS) --> icmp(X, 0)

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

LGTM.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:1958
+  // icmp(X | LHS, C) --> icmp(X, 0)
+  if (match(Or, m_c_Or(m_Value(X), m_APInt(LHS)))) {
+    if (C.isNonNegative()) {
----------------
nit: Your comments below use `RHS` but the variable is `LHS`. Can one of those be changed for consistencies sake?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:1959
+  if (match(Or, m_c_Or(m_Value(X), m_APInt(LHS)))) {
+    if (C.isNonNegative()) {
+      switch (Pred) {
----------------
goldstein.w.n wrote:
> nit: Can you make it:
> ```
> if (C.isNonNegative() && match(Or, m_c_Or(m_Value(X), m_APInt(LHS))))
> ```
> 
> Otherwise LGTM.
> nit: Can you make it:
> ```
> if (C.isNonNegative() && match(Or, m_c_Or(m_Value(X), m_APInt(LHS))))
> ```
> 
> Otherwise LGTM.

A few other nits as well.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:1984
+                              ConstantInt::getNullValue(X->getType()));
+        break;
+      default:
----------------
The `sge`/`slt` and the `sgt`/`sle` cases are indentical. Can you merge them i.e:

```
      // X | RHS s<= C --> X s< 0 iff LHS s> C s>= 0
      case ICmpInst::ICMP_SLE:
      // X | RHS s> C --> X s>= 0 iff LHS s> C s>= 0
      case ICmpInst::ICMP_SGT:
        if (LHS->sgt(C))
          return new ICmpInst(Pred, X,
                              ConstantInt::getNullValue(X->getType()));
        break;
      // X | RHS s< C --> X s< 0 iff LHS s>= C s>= 0
      case ICmpInst::ICMP_SLT:
      // X | RHS s>= C --> X s>= 0 iff LHS s>= C s>= 0
      case ICmpInst::ICMP_SGE:
        if (LHS->sge(C))
          return new ICmpInst(Pred, X,
                              ConstantInt::getNullValue(X->getType()));
        break;

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147597