[PATCH] D11146: [ValueTracking] Add support for floating-point minnum and maxnum

[Matt Arsenault]

arsenm added a subscriber: arsenm.

================
Comment at: lib/Analysis/ValueTracking.cpp:3325-3326
@@ +3324,4 @@
+    return !C->isNaN();
+  else
+    return false;
+}
----------------
No return after else

================
Comment at: lib/Analysis/ValueTracking.cpp:3332-3333
@@ +3331,4 @@
+    return !C->isZero();
+  else
+    return false;
+}
----------------
ditto

================
Comment at: lib/Analysis/ValueTracking.cpp:3360
@@ +3359,3 @@
+  SelectPatternNaNBehavior B = SPNB_NA;
+  bool O = false;
+
----------------
This could use a better name.

================
Comment at: lib/Analysis/ValueTracking.cpp:3403-3405
@@ +3402,5 @@
+    Pred = CmpInst::getSwappedPredicate(Pred);
+    if (B == SPNB_RETURNS_NAN) B = SPNB_RETURNS_OTHER;
+    else if (B == SPNB_RETURNS_OTHER) B = SPNB_RETURNS_NAN;
+    O = !O;
+  }
----------------
This indentation is confusing looking. The assignments should be on their own line, the O = !O looks like it's under the else if

================
Comment at: test/Transforms/InstCombine/minmax-fp.ll:139
@@ +138,3 @@
+define i8 @t15(float %a) {
+  %1 = fcmp nsz ule float %a, 0.0
+  %2 = fptosi float %a to i8
----------------
I don't think I see a test that explicitly checks for fcmp nnan

================
Comment at: unittests/Analysis/ValueTrackingTest.cpp:36
@@ +35,3 @@
+    if (!M)
+      report_fatal_error(os.str().c_str());
+
----------------
I don't think you need the .c_str() here


Repository:
  rL LLVM

http://reviews.llvm.org/D11146