[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: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();
----------------
This should be `if (match(Op1, m_APInt(COp1))` to match a constant int or int splat.


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



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


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3397
+        LogicalOp, Builder.CreateICmp(NewPred, Op0, Op1),
+        Builder.CreateICmp(Pred, Builder.CreateSub(Op0, Op1), CondConstant));
+  }
----------------
Do we ever actually take this fallback code patch? I searched for `freeze`, `and i1` and `or i1` in your tests without hits.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154206