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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


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


================
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()) {
----------------
I don't think this comment adds value relative to the old one-liner...


================
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))
----------------
I'd move this compare() call into foldICmpUSubSatWithConstant(). Seems odd to split this operation out.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3381
+        APInt EquivalentInt;
+        Combination->getEquivalentICmp(EquivalentPred, EquivalentInt);
+
----------------
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`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154206