[AArch64] Avoid materializing 0.0 when generating FP CSEL

[evandro]

Summary

Transform a == 0.0 ? 0.0 : x to a == 0.0 ? a : x and a != 0.0 ? x : 0.0 to a != 0.0 ? x : a to avoid materializing 0.0 for FCSEL, since it does not have to be materialized beforehand for FCMP, as it has a form that implies 0.0 as an operand.

[MatzeB]

Wouldn't this work with any constant? Should we consider it for the common DAGCombiner?

[evandro]

In D24808#548901, @MatzeB wrote:

Wouldn't this work with any constant? Should we consider it for the common DAGCombiner?

It might, but AArch64 doesn't materialize 0.0 explicitly when comparing against it (e.g., fcmp d0, #0.0), so it was my initial focus. Also, because it seems to be the most common value with CSEL.

I'll investigate it further.

[gberry]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

4083

Isn't the goal to check these options less and look at the DAG node flags instead?
If so, this might be better written as follows?

DAG.getFlags()->hasNoSignedZeros()

[evandro]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

4083

Yes, thank you!

[mcrosier]

Please include full context.

[evandro]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

4083

Am I missing something or SelectionDAG has no such method? Not all SDValue have SDNodeFlags either...

[evandro]

In D24808#548901, @MatzeB wrote:

Wouldn't this work with any constant? Should we consider it for the common DAGCombiner?

Thinking more about this suggestion, I think that this change improves code generation only if the comparison instruction has a form with an immediate operand encoding, which is very target dependent (e.g., AArch64). Otherwise, if the target has to materialize the constant for the comparison, it can be used again by the selection (e.g., x86).

I couldn't find or devise an method to query such conditions generically and I'd appreciate suggestions. If so, I believe that this change has to remain specific to AArch64 for this round.

[evandro]

Ping^1

[evandro]

Ping^2

[rengolin]

What about @MatzeB's suggestion to generalise it to any constant?

[evandro]

In D24808#571252, @rengolin wrote:

What about @MatzeB's suggestion to generalise it to any constant?

Please, see https://reviews.llvm.org/D24808#554492 and let me know if you need more information.

[evandro]

@spop

[sebpop]

In D24808#554492, @evandro wrote:

In D24808#548901, @MatzeB wrote:

Wouldn't this work with any constant? Should we consider it for the common DAGCombiner?

Thinking more about this suggestion, I think that this change improves code generation only if the comparison instruction has a form with an immediate operand encoding, which is very target dependent (e.g., AArch64). Otherwise, if the target has to materialize the constant for the comparison, it can be used again by the selection (e.g., x86).

LGTM. I think the patch looks good as it is.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

4058

The change from Evandro looks identical to this transform.
I don't see why we would need to do what Evandro proposes in the DAGcombine when this one is doing it at lowering.

[rengolin]

In D24808#571590, @evandro wrote:

Please, see https://reviews.llvm.org/D24808#554492 and let me know if you need more information.

That answers why not move into the DAGCombiner, not why not make it any constant, right?

[sebpop]

In D24808#573402, @rengolin wrote:

In D24808#571590, @evandro wrote:

Please, see https://reviews.llvm.org/D24808#554492 and let me know if you need more information.

That answers why not move into the DAGCombiner, not why not make it any constant, right?

That comment also has the answer for "why not make it any constant" as well. Here is what Evandro said:

if the target has to materialize the constant for the comparison, it can be used again by the selection

[evandro]

In D24808#573402, @rengolin wrote:

In D24808#571590, @evandro wrote:

Please, see https://reviews.llvm.org/D24808#554492 and let me know if you need more information.

That answers why not move into the DAGCombiner, not why not make it any constant, right?

I think that it answers both questions. Again, unless the target cam perform the comparison against a constant implicit in the corresponding instruction, then this transform provides no improvement. IOW, only if the target does not have to materialize the constant for the comparison does it make sense to not materialize it for the selection. Otherwise, if it has to be materialzied for one, it will likely be around to be used for the other.

[rengolin]

In D24808#573411, @evandro wrote:

I think that it answers both questions.

Well, that, and the fact that there's a special zero variant for fpcmp that I just found out. :)

I agree this is at least a place as good as any. I'm not sure how this could be done at the DAGCombiner level, being such target-specific transformation.

LGTM. Thanks!

[evandro]

In D24808#573438, @rengolin wrote:

Well, that, and the fact that there's a special zero variant for fpcmp that I just found out. :)

I stated as much in the patch summary above:

FCMP, as it has a form that implies 0.0 as an operand.

Thank you.

[rengolin]

In D24808#573459, @evandro wrote:

FCMP, as it has a form that implies 0.0 as an operand.

You did. Completely my fault. Sorry! :)

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

   // clean.  Some of them require two CSELs to implement.
   AArch64CC::CondCode CC1, CC2;
   changeFPCCToAArch64CC(CC, CC1, CC2);
+
+  if (DAG.getTarget().Options.UnsafeFPMath) {
+    // Transform "a == 0.0 ? 0.0 : x" to "a == 0.0 ? a : x" and
+    // "a != 0.0 ? x : 0.0" to "a != 0.0 ? x : a" to avoid materializing 0.0.
+    ConstantFPSDNode *RHSVal = dyn_cast<ConstantFPSDNode>(RHS);
+    if (RHSVal && RHSVal->isZero()) {
+      ConstantFPSDNode *CFVal = dyn_cast<ConstantFPSDNode>(FVal);
+      ConstantFPSDNode *CTVal = dyn_cast<ConstantFPSDNode>(TVal);
+
+      if ((CC == ISD::SETEQ || CC == ISD::SETOEQ || CC == ISD::SETUEQ) &&
+          CTVal && CTVal->isZero())
+        TVal = LHS;
+      else if ((CC == ISD::SETNE || CC == ISD::SETONE || CC == ISD::SETUNE) &&
+               CFVal && CFVal->isZero())
+        FVal = LHS;
+    }
+  }
+
+  // Emit first, and possibly only, CSEL.
   SDValue CC1Val = DAG.getConstant(CC1, dl, MVT::i32);
   SDValue CS1 = DAG.getNode(AArch64ISD::CSEL, dl, VT, TVal, FVal, CC1Val, Cmp);
 

llvm/test/CodeGen/AArch64/fcsel-zero.ll

+; Check that 0.0 is not materialized for CSEL when comparing against it.
+
+; RUN: llc -mtriple=aarch64 -o - < %s | FileCheck %s
+
+define float @foeq(float %a, float %b) #0 {
+  %t = fcmp oeq float %a, 0.0
+  %v = select i1 %t, float 0.0, float %b
+  ret float %v
+; CHECK-LABEL: foeq
+; CHECK: fcmp [[R:s[0-9]+]], #0.0
+; CHECK-NEXT: fcsel {{s[0-9]+}}, [[R]], {{s[0-9]+}}, eq
+}
+
+define float @fueq(float %a, float %b) #0 {
+  %t = fcmp ueq float %a, 0.0
+  %v = select i1 %t, float 0.0, float %b
+  ret float %v
+; CHECK-LABEL: fueq
+; CHECK: fcmp [[R:s[0-9]+]], #0.0
+; CHECK-NEXT: fcsel {{s[0-9]+}}, [[R]], {{s[0-9]+}}, eq
+; CHECK-NEXT: fcsel {{s[0-9]+}}, [[R]], {{s[0-9]+}}, vs
+}
+
+define float @fone(float %a, float %b) #0 {
+  %t = fcmp one float %a, 0.0
+  %v = select i1 %t, float %b, float 0.0
+  ret float %v
+; CHECK-LABEL: fone
+; CHECK: fcmp [[R:s[0-9]+]], #0.0
+; CHECK-NEXT: fcsel {{s[0-9]+}}, {{s[0-9]+}}, [[R]], mi
+; CHECK-NEXT: fcsel {{s[0-9]+}}, {{s[0-9]+}}, [[R]], gt
+}
+
+define float @fune(float %a, float %b) #0 {
+  %t = fcmp une float %a, 0.0
+  %v = select i1 %t, float %b, float 0.0
+  ret float %v
+; CHECK-LABEL: fune
+; CHECK: fcmp [[R:s[0-9]+]], #0.0
+; CHECK-NEXT: fcsel {{s[0-9]+}}, {{s[0-9]+}}, [[R]], ne
+}
+
+define double @doeq(double %a, double %b) #0 {
+  %t = fcmp oeq double %a, 0.0
+  %v = select i1 %t, double 0.0, double %b
+  ret double %v
+; CHECK-LABEL: doeq
+; CHECK: fcmp [[R:d[0-9]+]], #0.0
+; CHECK-NEXT: fcsel {{d[0-9]+}}, [[R]], {{d[0-9]+}}, eq
+}
+
+define double @dueq(double %a, double %b) #0 {
+  %t = fcmp ueq double %a, 0.0
+  %v = select i1 %t, double 0.0, double %b
+  ret double %v
+; CHECK-LABEL: dueq
+; CHECK: fcmp [[R:d[0-9]+]], #0.0
+; CHECK-NEXT: fcsel {{d[0-9]+}}, [[R]], {{d[0-9]+}}, eq
+; CHECK-NEXT: fcsel {{d[0-9]+}}, [[R]], {{d[0-9]+}}, vs
+}
+
+define double @done(double %a, double %b) #0 {
+  %t = fcmp one double %a, 0.0
+  %v = select i1 %t, double %b, double 0.0
+  ret double %v
+; CHECK-LABEL: done
+; CHECK: fcmp [[R:d[0-9]+]], #0.0
+; CHECK-NEXT: fcsel {{d[0-9]+}}, {{d[0-9]+}}, [[R]], mi
+; CHECK-NEXT: fcsel {{d[0-9]+}}, {{d[0-9]+}}, [[R]], gt
+}
+
+define double @dune(double %a, double %b) #0 {
+  %t = fcmp une double %a, 0.0
+  %v = select i1 %t, double %b, double 0.0
+  ret double %v
+; CHECK-LABEL: dune
+; CHECK: fcmp [[R:d[0-9]+]], #0.0
+; CHECK-NEXT: fcsel {{d[0-9]+}}, {{d[0-9]+}}, [[R]], ne
+}
+
+attributes #0 = { nounwind "unsafe-fp-math"="true" }
+