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

[Dhruv Chawla via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3369
+    ConstantRange C1 = ConstantRange::makeExactICmpRegion(NewPred, *COp1);
+    // Convert '(X - C) pred C2' into 'X pred (C + C2)'.
+    ConstantRange C2 = ConstantRange::makeExactICmpRegion(Pred, C);
----------------
nikic wrote:
> This comment isn't correct.
Is this better: `// Convert '(X - C) pred C2' into 'X pred C2' shifted by C.`?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3660
                                                              IntrinsicInst *II,
                                                              const APInt &C) {
   if (Cmp.isEquality())
----------------
nikic wrote:
> In terms of overall code structure, I would prefer it if we added an extra `switch (II->getIntrinsicID())` here, before the isEquality() handling, where we can handle folds that apply for both equality and non-equality comparisons. This is a recurring pattern, e.g. we've seen the same issue in D152677.
Done!


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3381
+        APInt EquivalentInt;
+        Combination->getEquivalentICmp(EquivalentPred, EquivalentInt);
+
----------------
0xdc03 wrote:
> 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?
I marked this as done, but I think its worth confirming this just in case you missed this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154206