[PATCH] D65765: [InstCombine] Non-canonical clamp-like pattern handling

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1127
+//   %r = select i1 %old_cmp0, i32 %x, i32 %old_replacement
+// This can be rewriten as more canonical pattern:
+//   %new_cmp1 = icmp slt i32 %x, -C1
----------------
rewriten -> rewritten


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1133-1134
+// Iff -C1 s<= C2 s<= C0-C1
+// Also, ULT predicate can also be UGE; or UGT iff C0 != -1 (+invert result)
+// Also, SLT predicate can also be SGE; or SGT iff C2 != INT_MAX (+invert res.)
+static Instruction *canonicalizeClampLike(SelectInst &Sel0, ICmpInst &Cmp0,
----------------
'Also/also' repeated in each statement; can remove 1 or the other.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1169-1170
+    LLVM_FALLTHROUGH;
+  case ICmpInst::Predicate::ICMP_UGE:
+    // Also non-canonical, but here we don't need to change C0,
+    // so we don't have any restrictions on C0, so we can just handle it.
----------------
There is no test with UGE predicate? Does this require a non-splat vector constant?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1180
+  // Now that we've canonicalized the ICmp, we know the X we expect;
+  // the select in other hand shold be one-use.
+  if (!Sel1->hasOneUse())
----------------
shold -> should


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1206
+  if (!Cmp1->hasOneUse() && (Cmp00 == X || !Cmp00->hasOneUse()))
+    return nullptr; // Not enough one-use instructions for the fold. :/
+  // FIXME: this restriction could be relaxed if Cmp1 can be reused as one of
----------------
Remove ':/' - doesn't add value; might cause confusion for some readers.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1230-1231
+    LLVM_FALLTHROUGH;
+  case ICmpInst::Predicate::ICMP_SGE:
+    // Also non-canonical, but here we don't need to change C2,
+    // so we don't have any restrictions on C2, so we can just handle it.
----------------
There is no test with SGE predicate? Does this require a non-splat vector constant?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65765