[PATCH] D154206: [InstCombine] Fold comparison of usub.sat

[Dhruv Chawla via Phabricator via llvm-commits]

0xdc03 marked 7 inline comments as done.
0xdc03 added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3438
+      Value *II1 = II->getOperand(1);
+      ICmpInst::Predicate EqPred =
+          Pred == ICmpInst::ICMP_EQ ? ICmpInst::ICMP_UGE : ICmpInst::ICMP_ULT;
----------------
goldstein.w.n wrote:
> `EqPred` is kind of confusing (since its a non-equality pred), maybe `NewPred`?
Done!


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3440
+          Pred == ICmpInst::ICMP_EQ ? ICmpInst::ICMP_UGE : ICmpInst::ICMP_ULT;
+      if (match(II1, m_Constant()))
+        return BinaryOperator::CreateAnd(
----------------
nikic wrote:
> m_ImmConstant
Done!


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3672
+    Value *II0 = II->getOperand(0);
+    Value *II1 = II->getOperand(1);
+    if (match(II1, m_Constant())) {
----------------
nikic wrote:
> Add comment explaining the transform, something like this:
> ```
> //    usub.sat(X, C) pred C2
> // -> (X < C ? 0 : X - C) pred C2
> // -> X >= C && (X - C) pred C2 (if 0 pred C2 is false)
> // -> X < C || (X - C) pred C2 (if 0 pred C2 is true)
> ```
Done!


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3673
+    Value *II1 = II->getOperand(1);
+    if (match(II1, m_Constant())) {
+      if (ICmpInst::compare(APInt::getZero(C.getBitWidth()), C, Pred))
----------------
goldstein.w.n wrote:
> `m_ImmConstant()` here as well.
Done!


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3682
+          Builder.CreateICmp(Pred, Builder.CreateSub(II0, II1), Builder.getInt(C)));
+    }
+    break;
----------------
nikic wrote:
> Some of these cases will end up producing 2 instructions (add + icmp), so I think we should add a one-use limitation on the intrinsic.
Done!


================
Comment at: llvm/test/Transforms/InstCombine/icmp-usub-sat.ll:120
+
+declare i8 @llvm.usub.sat.i8(i8, i8)
----------------
goldstein.w.n wrote:
> nikic wrote:
> > Add a multi-use and vector test.
> Can you split tests to seperate patch (make series with tests as Patch1 then impl as Patch2) so we can see the diff this patch causes.
Done! D154342


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154206