[PATCH] D65017: [InstCombine] Teach foldOrOfICmps to allow icmp eq MIN_INT/MAX to be part of a range comparision. Similar for foldAndOfICmps

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1208-1211
+      // (X != INT_MIN & X s< 14) -> X-(INT_MIN+1) u< (14-(INT_MIN+1))
+      if (LHSC->isMinValue(true))
+        return insertRangeTest(LHS0, LHSC->getValue() + 1, RHSC->getValue(),
+                               true, true);
----------------
Ok, https://rise4fun.com/Alive/VUp
Comment does not specify the precondition that `C != INT_MIN`
I'm guessing that case is handled elsewhere?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1226-1229
+      // X u> 13 & X != UINT_MAX -> (X-14) u< UINT_MAX-14
+      if (RHSC->isMaxValue(false))
+        return insertRangeTest(LHS0, LHSC->getValue() + 1, RHSC->getValue(),
+                               false, true);
----------------
Update comment please: `X u> 13 & X != UINT_MAX -> (X-(C+1)) u< UINT_MAX-(C+1)`
https://rise4fun.com/Alive/qt2, ok
Comment does not specify the precondition that `C != UINT_MAX`
I'm guessing that case is handled elsewhere?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1244-1247
+      // X s> 13 & X != INT_MAX -> (X-14) u< INT_MAX-14
+      if (RHSC->isMaxValue(true))
+        return insertRangeTest(LHS0, LHSC->getValue() + 1, RHSC->getValue(),
+                               true, true);
----------------
Fix comment please: `X s> 13 & X != INT_MAX -> (X-(C+1))) u< INT_MAX-(C+1)`
https://rise4fun.com/Alive/hZlO, ok
Comment does not specify the precondition that `C != INT_MAX`
I'm guessing that case is handled elsewhere?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2269-2309
+      // We can treat equality of UINT_MIN the same as ULE UINT_MIN.
+      if (LHSC->isMinValue(false))
+        return insertRangeTest(LHS0, LHSC->getValue() + 1, RHSC->getValue() + 1,
+                               false, false);
+      // (X == 13 | X u> 14) -> no change
+      break;
+    case ICmpInst::ICMP_SGT:
----------------
Can you please add actual formulas here as comments?


================
Comment at: llvm/test/Transforms/InstCombine/and-or-icmps.ll:316
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp ugt i32 [[X_OFF]], 2147483645
+; CHECK-NEXT:    ret i1 [[TMP1]]
 ;
----------------
craig.topper wrote:
> nikic wrote:
> > This seems like a weird choice that the range check code is making. We could also generate
> > 
> > ```
> > %x.off = add i32 %x, 1
> > %c = icmp ult i32 %x.off, -2147483646
> > ```
> > 
> > with smaller constants.
> Is this something this patch should address?
> Is this something this patch should address?

I'm going to go with no. That is a separate fold.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65017