[PATCH] D139130: InstCombine: Fold and (fcmp), (is.fpclass) into is.fpclass

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1259
+    switch (Pred) {
+#if 0
+    // TODO: Compares with 0 depends on the denormal handling mode.
----------------
Remove the `#if 0` block, just leave a TODO.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1271
+    case FCmpInst::FCMP_ORD:
+      // Canonical form of ord/uno is with a zero. We would also handle
+      // non-canonical other non-NaN constants or LHS == RHS.
----------------
What does "would" mean here?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1298
+      //   fcmp oeq x, -inf -> is_fpclass x, fcNegInf
+      //   fcmp oeq fabs(x), -inf -> is_fpclass x, ~fcNan
+      //
----------------
Should be `is_fpclass x, 0` aka `false`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1303
+      //   fcmp une x, -inf -> is_fpclass x, ~fcNegInf
+      //   fcmp une fabs(x), -inf -> is_fpclass x, fcNan
+
----------------
Should be `is_fpclass x, ~0` aka `true`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1308
+        if (IsFabs)
+          Mask = ~fcNan; // FIXME: Can't write a test
+      } else {
----------------
Should be `Mask = 0`.

Can you imagine any way of fixing the FIXME? If not I wouldn't bother marking it as FIXME.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1349
+      // fcmp uge fabs(x), +inf -> ~fcFinite
+      // fcmp olt x, +inf -> fcNegInf|fcSubnormal|fcNormal|fcZero
+      // fcmp uge x, +inf -> ~(fcFinite|fcNegInf)
----------------
Simpler to write this as `fcFinite|fcNegInf`, as on the line below.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1522-1523
+  if (((IsLHSClass && IsRHSClass) ||
+       ((!IsLHSClass && matchIsFPClassLikeFCmp(Op0, ClassVal0, ClassMask0)) ||
+        (!IsRHSClass && matchIsFPClassLikeFCmp(Op1, ClassVal1, ClassMask1)))) &&
+      ClassVal0 == ClassVal1) {
----------------
Can remove one of the many sets of parens here.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1319-1321
+      // Match __builtin_isinf patterns
+      //   fcmp one x, -inf -> is_fpclass x, fcNegInf
+      //   fcmp one fabs(x), -inf -> is_fpclass x, ~fcNan
----------------
jcranmer-intel wrote:
> 
Agree with @jcranmer-intel. It looks like you might have "fixed" the wrong line here?


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

https://reviews.llvm.org/D139130