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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D139130#3995198 <https://reviews.llvm.org/D139130#3995198>, @sepavloff wrote:

> 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.

If you're doing 2 or more checks I think is.fpclass is a better canonical form so it doesn't matter what the target prefers, that's a lowering issue. This patch holds off on introducing new classes because we have a bit of codegen improvement to do. I don't think that should block this patch,
since if you're already using class you've already signed up for not-ideal codegen.

> 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.

We already have fpclass legalization tests. I'll add more when I get to the codegen improvements (mostly we're missing all the cases where it can be converted back to FP compares, it only handles a couple of them. Plus it's mishandling denormals with DAZ)



================
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};
----------------
sepavloff wrote:
> 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.
I don't know if it really should. The more non-canonical forms we try to match, the more edge cases that we won't actually encounter and won't have testing for.


================
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
----------------
sepavloff wrote:
> Is this comparison always false?
No, this is the same as fcmp ord


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

https://reviews.llvm.org/D139130