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

[Dhruv Chawla via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3363-3366
+  if (match(Op1, m_ImmConstant(COp1))) {
+    // Try to see if we can generate a single icmp directly.
+    if (isa<ConstantInt>(COp1) || COp1->getSplatValue()) {
+      const APInt &COp1Int = COp1->getUniqueInteger();
----------------
nikic wrote:
> This should be `if (match(Op1, m_APInt(COp1))` to match a constant int or int splat.
Done!


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3378
+
+      if (Combination.has_value()) {
+        CmpInst::Predicate EquivalentPred;
----------------
nikic wrote:
> 
Done!


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3381
+        APInt EquivalentInt;
+        Combination->getEquivalentICmp(EquivalentPred, EquivalentInt);
+
----------------
nikic wrote:
> Check the return value -- does this actually always succeed?
> 
> It would have expected that you need to use the getEquivalentICmp overload that also provides an additional Offset (because that one *is* guaranteed to succeed).
Done! I am a bit concerned about the codegen this causes, maybe I have done something wrong with using `Builder.CreateAdd`? Looking at test cases like `icmp_sle_basic_positive`, there is an extra add instruction generated, which I would've expected to get folded. Could this be due to a lack of nuw/nsw?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3397
+        LogicalOp, Builder.CreateICmp(NewPred, Op0, Op1),
+        Builder.CreateICmp(Pred, Builder.CreateSub(Op0, Op1), CondConstant));
+  }
----------------
nikic wrote:
> Do we ever actually take this fallback code patch? I searched for `freeze`, `and i1` and `or i1` in your tests without hits.
Yeah, I had not come up with a test case to exercise that code path, because I didn't realize that the fold is only called when splat vectors are given as operands to the icmp, and I have no tests to check non-splat vector in the `usub.sat` and a splat vector in the icmp.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3498
+    if (!II->hasOneUse())
+      break;
+
----------------
nikic wrote:
> Could fold this into foldICmpUSubSatWithConstant() and only impose the limitation if we actually need to produce more than one instruction.
Done!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154206