[PATCH] D33186: [InstCombine] Canonicalize clamp of float types to minmax in fast mode.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:3948
+  // Try to match X `pred` C1 ? C1 : ProperMinMax(X, C2) into
+  // MaxMin(C1, ProperMinMax(X, C2) and return description of the
+  // outer one.
----------------
Missing close-paren?  Also, MaxMin and MinMax are a little confusing; just write out one, I think


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:3951
+
+  // First, check if select has inversed order:
+  if (CmpRHS == FalseVal) {
----------------
"inversed" isn't a word.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:3957
+
+  // Assume success now. If there's no match, callers should not use these anyway.
+  LHS = TrueVal;
----------------
"Assume success" is a little confusing... maybe something more like "Return the LHS and RHS early".


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4206
+      (!FMF.noSignedZeros() && !isKnownNonZero(CmpLHS) &&
+       !isKnownNonZero(CmpRHS)))
+    return {SPF_UNKNOWN, SPNB_NA, false};
----------------
Need an explanation here describing why signed zeros matter.  (Something about the sign of the result?)


================
Comment at: llvm/test/Transforms/InstCombine/clamp-to-minmax.ll:146
 ; (X > 1.0) ? min(x, 255.0) : 1.0
+; That dit not match because select was in inversed order
 define float @clamp_test_1(float %x) {
----------------
"dit"?


https://reviews.llvm.org/D33186