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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added a comment.

Some mechanism must exists that applies this transformation or not depending on target. If instruction FCLASS is available, the transformation is profitable. But on targets that do not have such instruction, the replacement may produce inefficient code.

Also it is desirable to have a codegen test for target without FCLASS instruction (like x86), to check if the code generated for such comparisons does not become worse.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1231-1236
+    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.
+      return {LHS, ~fcNan & fcAllFlags};
+    case FCmpInst::FCMP_UNO:
+      return {LHS, fcNan};
----------------
These checks should be made for more general case:

* LHS is same as RHS,
* RHS is a constant,

as you mention is the comment above.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1257
+      //   fcmp oeq x, +inf -> is_fpclass x, fcPosInf
+      //   fcmp ueq x, +inf -> is_fpclass x, fcPosInf|fcNan
+      //   fcmp oeq fabs(x), +inf -> is_fpclass x, fcInf
----------------
Is this comment relevant? This block executes for `une`, not `ueq`?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1263
+      //   fcmp ueq x, -inf -> is_fpclass x, fcNegInf|fcNan
+      //   fcmp oeq fabs(x), -inf -> is_fpclass x, ~fcNan
+      //   fcmp ueq fabs(x), -inf -> is_fpclass x, fcNan
----------------
Is this comparison always false?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1282
+      //
+      //   fcmp oeq x, +inf -> is_fpclass x, fcPosInf
+      //   fcmp oeq fabs(x), +inf -> is_fpclass x, fcInf
----------------
`oeq` seems irrelevant in this block.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1323-1324
+
+      // fcmp oge fabs(x), +inf -> fcPosInf
+      // fcmp oge x, +inf -> fcInf
+      // fcmp ult fabs(x), +inf -> ~fcPosInf
----------------



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1325-1326
+      // fcmp oge x, +inf -> fcInf
+      // fcmp ult fabs(x), +inf -> ~fcPosInf
+      // fcmp ult x, +inf -> ~fcInf
+      Mask = fcPosInf;
----------------



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1340
+    case FCmpInst::FCMP_UGE: {
+      // fcmp olt x, smallest_normal -> fcNegInf|fcSubnormal|fcZero
+      // fcmp olt fabs(x), smallest_normal -> fcSubnormal|fcZero
----------------



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1342
+      // fcmp olt fabs(x), smallest_normal -> fcSubnormal|fcZero
+      // fcmp uge x, smallest_normal -> ~(fcZero|fcSubnormal)
+      // fcmp uge fabs(x), smallest_normal -> ~(fcSubnormal|fcZero)
----------------



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

https://reviews.llvm.org/D139130