[PATCH] D154206: [InstCombine] Fold comparison of usub.sat
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 4 07:26:54 PDT 2023
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
More information about the llvm-commits
mailing list