[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:3366
+
+  const APInt *COp1 = nullptr;
+  if (match(Op1, m_APInt(COp1))) {
----------------



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3367
+  const APInt *COp1 = nullptr;
+  if (match(Op1, m_APInt(COp1))) {
+    ConstantRange C1 = ConstantRange::makeExactICmpRegion(NewPred, *COp1);
----------------
Prefer early return over nesting.


================
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);
----------------
This comment isn't correct.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3373
+
+    std::optional<ConstantRange> Combination = std::nullopt;
+    if (LogicalOp == Instruction::BinaryOps::Or)
----------------



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3379
+
+    if (Combination) {
+      CmpInst::Predicate EquivPred;
----------------
Early return


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3660
                                                              IntrinsicInst *II,
                                                              const APInt &C) {
   if (Cmp.isEquality())
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154206