Allow fneg + strict_fadd -> strict_fsub in DAGCombiner

[ajwock]

Summary

This is the first of a set of DAGCombiner changes enabling strictfp optimizations. I want to test to waters with this to make sure changes like these are acceptable for the strictfp case- this particular change should preserve exception ordering and result precision perfectly, and many other possible changes appear to be able to as well.

Copied from regular fadd combines but modified to preserve ordering via the chain, this change allows strict_fadd x, (fneg y) to become strict_fsub x, y and strict_fadd (fneg x), y to become strict_fsub y, x.

[arsenm]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12824

There is some strange legality predicate you are supposed to use for the strict cases, although I don't fully understand it

[kpn]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12824

Where is this predicate? Is there an existing case that can be modeled after?

My thinking is that this transform is probably safe because the FNeg will never trap and it will never round so it should therefore be safe to fold it into the add/sub. But I'm no expert on floating-point I admit.

[arsenm]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12824

This is totally valid. But for some reason, we have getStrictFPOperationAction which differs from checking the regular legalization action on the STRICT_ opcode

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12824

I think getStrictFPOperationAction works along with the code in SelectionDAGISel that mutates strict nodes to non-strict nodes for targets that haven't done the work. It was intended to query whether the target will be able to handle the mutated node. It should go away when all targets have done the work.

Since I think we're now blocking clang from using strict fp on targets that haven't done the work. Then we probably can just use the normal legal operation check.

[ajwock]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12824

As far as I can tell, that comes into play later on as craig topper has stated. I don't think there's any way for that to apply during combination. Here we have to rely on operations that we can be sure are legal. As long as this operation is legal according to the standard (I'm also no expert, but I'm pretty sure based on my interpretation) exceptions should still occur as the original code prescribes, and in the same order as such.

[spatel]

Why wait until codegen to do this? IIUC, it's impossible for any backend to spontaneously create strict FP ops, so this pattern must be present in the original IR.
Ie, wouldn't we be better off reducing these in IR (InstCombinerImpl::visitCallInst())?

[arsenm]

In D85548#2223906, @spatel wrote:

Why wait until codegen to do this? IIUC, it's impossible for any backend to spontaneously create strict FP ops, so this pattern must be present in the original IR.
Ie, wouldn't we be better off reducing these in IR (InstCombinerImpl::visitCallInst())?

It doesn't matter if instcombine does this or not, this pattern could still easily appear after legalization

[spatel]

In D85548#2223912, @arsenm wrote:

In D85548#2223906, @spatel wrote:

Why wait until codegen to do this? IIUC, it's impossible for any backend to spontaneously create strict FP ops, so this pattern must be present in the original IR.
Ie, wouldn't we be better off reducing these in IR (InstCombinerImpl::visitCallInst())?

It doesn't matter if instcombine does this or not, this pattern could still easily appear after legalization

Ok, I haven't followed strict FP closely, so my assumption was wrong. Is there a test or example somewhere that shows how a strict fadd is created out of something else?

[arsenm]

In D85548#2223973, @spatel wrote:

In D85548#2223912, @arsenm wrote:

In D85548#2223906, @spatel wrote:

Why wait until codegen to do this? IIUC, it's impossible for any backend to spontaneously create strict FP ops, so this pattern must be present in the original IR.
Ie, wouldn't we be better off reducing these in IR (InstCombinerImpl::visitCallInst())?

It doesn't matter if instcombine does this or not, this pattern could still easily appear after legalization

Ok, I haven't followed strict FP closely, so my assumption was wrong. Is there a test or example somewhere that shows how a strict fadd is created out of something else?

You may need to expand strict FP operations in terms of other strict FP operations. If I were to seriously try to implement strictfp for AMDGPU, it would look like a lot of expansions with other strict operations. I think this is too much effort for SelectionDAG, but is more manageable for GlobalISel.

Also, in general, codegen may need more relaxed rules for mixing strict and non-strict FP operations (at least in GlobalISel where there's an explicit instruction ordering). For example, AMDGPU's non-strict fdiv lowering needs to introduce mode switches (which in the DAG we use custom strict FP nodes with glue)

[ajwock]

This combine does create the strict_fsub in terms of strict_fadd while preserving the chain-protected ordering. Would it still be appropriate to have this transform and others like it in the DAGCombiner?

[arsenm]

LGTM

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

private:
     SDValue visitSIGN_EXTEND_INREG(SDNode *N);
     SDValue visitSIGN_EXTEND_VECTOR_INREG(SDNode *N);
     SDValue visitZERO_EXTEND_VECTOR_INREG(SDNode *N);
     SDValue visitTRUNCATE(SDNode *N);
     SDValue visitBITCAST(SDNode *N);
     SDValue visitFREEZE(SDNode *N);
     SDValue visitBUILD_PAIR(SDNode *N);
     SDValue visitFADD(SDNode *N);
+    SDValue visitSTRICT_FADD(SDNode *N);
     SDValue visitFSUB(SDNode *N);
     SDValue visitFMUL(SDNode *N);
     SDValue visitFMA(SDNode *N);
     SDValue visitFDIV(SDNode *N);
     SDValue visitFREM(SDNode *N);
     SDValue visitFSQRT(SDNode *N);
     SDValue visitFCOPYSIGN(SDNode *N);
     SDValue visitFPOW(SDNode *N);
SDValue DAGCombiner::visit(SDNode *N) {
   case ISD::AssertAlign:        return visitAssertAlign(N);
   case ISD::SIGN_EXTEND_INREG:  return visitSIGN_EXTEND_INREG(N);
   case ISD::SIGN_EXTEND_VECTOR_INREG: return visitSIGN_EXTEND_VECTOR_INREG(N);
   case ISD::ZERO_EXTEND_VECTOR_INREG: return visitZERO_EXTEND_VECTOR_INREG(N);
   case ISD::TRUNCATE:           return visitTRUNCATE(N);
   case ISD::BITCAST:            return visitBITCAST(N);
   case ISD::BUILD_PAIR:         return visitBUILD_PAIR(N);
   case ISD::FADD:               return visitFADD(N);
+  case ISD::STRICT_FADD:        return visitSTRICT_FADD(N);
   case ISD::FSUB:               return visitFSUB(N);
   case ISD::FMUL:               return visitFMUL(N);
   case ISD::FMA:                return visitFMA(N);
   case ISD::FDIV:               return visitFDIV(N);
   case ISD::FREM:               return visitFREM(N);
   case ISD::FSQRT:              return visitFSQRT(N);
   case ISD::FCOPYSIGN:          return visitFCOPYSIGN(N);
   case ISD::FPOW:               return visitFPOW(N);
SDValue DAGCombiner::visitFADD(SDNode *N) {
   // FADD -> FMA combines:
   if (SDValue Fused = visitFADDForFMACombine(N)) {
     AddToWorklist(Fused.getNode());
     return Fused;
   }
   return SDValue();
 }
 
+SDValue DAGCombiner::visitSTRICT_FADD(SDNode *N) {
+  SDValue Chain = N->getOperand(0);
+  SDValue N0 = N->getOperand(1);
+  SDValue N1 = N->getOperand(2);
+  EVT VT = N->getValueType(0);
+  EVT ChainVT = N->getValueType(1);
+  SDLoc DL(N);
+  const SDNodeFlags Flags = N->getFlags();
+
+  // fold (strict_fadd A, (fneg B)) -> (strict_fsub A, B)
+  if (!LegalOperations || TLI.isOperationLegalOrCustom(ISD::STRICT_FSUB, VT))
+    if (SDValue NegN1 = TLI.getCheaperNegatedExpression(
+            N1, DAG, LegalOperations, ForCodeSize)) {
+      return DAG.getNode(ISD::STRICT_FSUB, DL, DAG.getVTList(VT, ChainVT),
+                         {Chain, N0, NegN1}, Flags);
+    }
+
+  // fold (strict_fadd (fneg A), B) -> (strict_fsub B, A)
+  if (!LegalOperations || TLI.isOperationLegalOrCustom(ISD::STRICT_FSUB, VT))
+    if (SDValue NegN0 = TLI.getCheaperNegatedExpression(
+            N0, DAG, LegalOperations, ForCodeSize)) {
+      return DAG.getNode(ISD::STRICT_FSUB, DL, DAG.getVTList(VT, ChainVT),
+                         {Chain, N1, NegN0}, Flags);
+    }
+  return SDValue();
+}
+
 SDValue DAGCombiner::visitFSUB(SDNode *N) {
   SDValue N0 = N->getOperand(0);
   SDValue N1 = N->getOperand(1);
   ConstantFPSDNode *N0CFP = isConstOrConstSplatFP(N0, true);
   ConstantFPSDNode *N1CFP = isConstOrConstSplatFP(N1, true);
   EVT VT = N->getValueType(0);
   SDLoc DL(N);
   const TargetOptions &Options = DAG.getTarget().Options;

llvm/test/CodeGen/AMDGPU/strict_fadd.f32.ll

; GCN-NEXT:    s_setpc_b64 s[30:31]
   %val = call float @llvm.experimental.constrained.fadd.f32(float %x, float %fabs.y, metadata !"round.tonearest", metadata !"fpexcept.strict")
   ret float %val
 }
 
 define float @v_constained_fadd_f32_fpexcept_strict_fneg_fabs_lhs(float %x, float %y) #0 {
 ; GCN-LABEL: v_constained_fadd_f32_fpexcept_strict_fneg_fabs_lhs:
 ; GCN:       ; %bb.0:
 ; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; GCN-NEXT:    v_add_f32_e64 v0, -|v0|, v1
+; GCN-NEXT:    v_sub_f32_e64 v0, v1, |v0|
 ; GCN-NEXT:    s_setpc_b64 s[30:31]
   %fabs.x = call float @llvm.fabs.f32(float %x)
   %neg.fabs.x = fneg float %fabs.x
   %val = call float @llvm.experimental.constrained.fadd.f32(float %neg.fabs.x, float %y, metadata !"round.tonearest", metadata !"fpexcept.strict")
   ret float %val
 }
 
 declare float @llvm.fabs.f32(float) #1
 declare float @llvm.experimental.constrained.fadd.f32(float, float, metadata, metadata) #1
 declare <2 x float> @llvm.experimental.constrained.fadd.v2f32(<2 x float>, <2 x float>, metadata, metadata) #1
 declare <3 x float> @llvm.experimental.constrained.fadd.v3f32(<3 x float>, <3 x float>, metadata, metadata) #1
 
 attributes #0 = { strictfp }
 attributes #1 = { inaccessiblememonly nounwind willreturn }

llvm/test/CodeGen/X86/strict-fadd-combines.ll

+; RUN: llc < %s -mtriple=x86_64-unknown-unknown | FileCheck %s
+
+define float @fneg_strict_fadd_to_strict_fsub(float %x, float %y) {
+  ; CHECK: subss %{{.*}}, %{{.*}}
+  ; CHECK-NEXT: retq
+  %neg = fneg float %y
+  %add = call float @llvm.experimental.constrained.fadd.f32(float %x, float %neg, metadata!"round.dynamic", metadata!"fpexcept.strict")
+  ret float %add
+}
+
+define float @fneg_strict_fadd_to_strict_fsub_2(float %x, float %y) {
+  ; CHECK: subss %{{.*}}, %{{.*}}
+  ; CHECK-NEXT: retq
+  %neg = fneg float %y
+  %add = call float @llvm.experimental.constrained.fadd.f32(float %neg, float %x, metadata!"round.dynamic", metadata!"fpexcept.strict")
+  ret float %add
+}
+
+define double @fneg_strict_fadd_to_strict_fsub_d(double %x, double %y) {
+  ; CHECK: subsd %{{.*}}, %{{.*}}
+  ; CHECK-NEXT: retq
+  %neg = fneg double %y
+  %add = call double @llvm.experimental.constrained.fadd.f64(double %x, double %neg, metadata!"round.dynamic", metadata!"fpexcept.strict")
+  ret double %add
+}
+
+define double @fneg_strict_fadd_to_strict_fsub_2d(double %x, double %y) {
+  ; CHECK: subsd %{{.*}}, %{{.*}}
+  ; CHECK-NEXT: retq
+  %neg = fneg double %y
+  %add = call double @llvm.experimental.constrained.fadd.f64(double %neg, double %x, metadata!"round.dynamic", metadata!"fpexcept.strict")
+  ret double %add
+}
+
+
+declare float @llvm.experimental.constrained.fadd.f32(float, float, metadata, metadata)
+declare double @llvm.experimental.constrained.fadd.f64(double, double, metadata, metadata)