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

[Dhruv Chawla via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3396
+
+  return nullptr;
+}
----------------
nikic wrote:
> Redundant nullptr return...
Oof ... :facepalm:


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3503
+    // - usub.sat(a, b) != c -> (a < b)  || (a - b) != c (as 0 != c is true)
+    // which is handled by foldICmpUSubSatWithConstant.
     if (C.isZero()) {
----------------
nikic wrote:
> I don't think this comment adds value relative to the old one-liner...
Removed it!


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3667
+    if (auto *Folded = foldICmpUSubSatWithConstant(
+            ICmpInst::compare(APInt::getZero(C.getBitWidth()), C, Pred), Pred,
+            II, C, Builder))
----------------
nikic wrote:
> I'd move this compare() call into foldICmpUSubSatWithConstant(). Seems odd to split this operation out.
Ah, that was bad copy pasting from the older code! Sorry about that.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3381
+        APInt EquivalentInt;
+        Combination->getEquivalentICmp(EquivalentPred, EquivalentInt);
+
----------------
nikic wrote:
> 0xdc03 wrote:
> > 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.
> The codegen here is correct. This is how you do a two-sided range check (it's equivalent to something like `X >= C1 && X <= C2`).
Ah, fair enough them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154206