[DAGCombiner] Propagate FMF flags in FMA folding

[qiucf]

Summary

DAG combiner folds (fma a 1.0 b) into (fadd a b) but the flag isn't propagated into new fadd. This patch fixes that.

---

However, besides this, SDNodeFlags are easily missed/passed wrongly. (D86871) Is there any 'rule' about whether pass flags or not? Roughly speaking, there are (or more than?) three types of transformation in SDAG:

• The node is eliminated. Flags of it will never be passed to others.
• A node is expanded/simplified into another node(s). In this case, flags of the node should be passed to all newly created node(s).
• Some nodes are transformed to another group of nodes. Here new nodes should have intersection of original flags? Or decided by opcode type?

We can do some check against new nodes or pass flags automatically if the rule is clear. Any ideas?

[spatel]

However, besides this, SDNodeFlags are easily missed/passed wrongly. (D86871) Is there any 'rule' about whether pass flags or not? Roughly speaking, there are (or more than?) three types of transformation in SDAG:

1. The node is eliminated. Flags of it will never be passed to others.
2. A node is expanded/simplified into another node(s). In this case, flags of the node should be passed to all newly created node(s).
3. Some nodes are transformed to another group of nodes. Here new nodes should have intersection of original flags? Or decided by opcode type?

We can do some check against new nodes or pass flags automatically if the rule is clear. Any ideas?

This is out-of-scope for this particular patch review, but:
The first 2 cases seem straightforward, although there is a possibility that a node that carries FMF can be converted into some other node that does not have any meaningful use of that flag (for example fdiv arcp becomes fmul arcp - should always be harmless?).
The third case is where things are not specified clearly.
In IR, we have class FastMathFlagGuard to save/carry/restore state within IRBuilder. Maybe SDAG could use something like that too?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

13191

If this block is dead code, you can remove it independently of this patch.

13194

Add a test (maybe with vector types) for this code path?

[nemanjai]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

13156

A couple of things are unclear to me (although not related to this patch):

1. Why do we handle the 0.0 and 1.0 special cases before canonicalization and thereby have to check both ways?
2. Why is the handling for -1.0 not in the same place?

13194

+1

[qiucf]

• Update tests
• Keep redundant code (irrelevant to this patch)

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

13156

The canonicalization code was added years after original folding logic. I guess it's just missed carelessly. We can have an NFC patch later refactoring it.

[spatel]

llvm/test/CodeGen/PowerPC/fma-combine.ll

293

I can't tell if anything is changing on this test with this patch. Should this not reduce to 'xor' like the scalar code?

[qiucf]

llvm/test/CodeGen/PowerPC/fma-combine.ll

293

For the NOVSX check: v2f64 isn't legal, so result before this patch is fsub+fsub. Now it returns constants.

If VSX is enabled, combiner will not meet the condition, since ConstantFPSDNode not works for vectors. So result doesn't change.

[spatel]

LGTM - This patch shows a minimal improvement, and I don't think it does any harm.
We should, however, follow-up with the clean-up of redundant/dead code and enhancement for vector support.

llvm/test/CodeGen/PowerPC/fma-combine.ll

293

Ah, ok. Please put a 'TODO' comment in the code and here in the test so we can remember that.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

     return DAG.getNode(ISD::FMA, DL, VT, NegN0, NegN1, N2, Flags);
 
   if (UnsafeFPMath) {
     if (N0CFP && N0CFP->isZero())
       return N2;
     if (N1CFP && N1CFP->isZero())
       return N2;
   }
-  // TODO: The FMA node should have flags that propagate to these nodes.
+
   if (N0CFP && N0CFP->isExactlyValue(1.0))
-    return DAG.getNode(ISD::FADD, SDLoc(N), VT, N1, N2);
+    return DAG.getNode(ISD::FADD, SDLoc(N), VT, N1, N2, Flags);
   if (N1CFP && N1CFP->isExactlyValue(1.0))
-    return DAG.getNode(ISD::FADD, SDLoc(N), VT, N0, N2);
+    return DAG.getNode(ISD::FADD, SDLoc(N), VT, N0, N2, Flags);
 
   // Canonicalize (fma c, x, y) -> (fma x, c, y)
   if (isConstantFPBuildVectorOrConstantFP(N0) &&
      !isConstantFPBuildVectorOrConstantFP(N1))
     return DAG.getNode(ISD::FMA, SDLoc(N), VT, N1, N0, N2);
 
   if (UnsafeFPMath) {
     // (fma x, c1, (fmul x, c2)) -> (fmul x, c1+c2)

       return DAG.getNode(ISD::FMA, DL, VT,
                          N0.getOperand(0),
                          DAG.getNode(ISD::FMUL, DL, VT, N1, N0.getOperand(1),
                                      Flags),
                          N2);
     }
   }
 
-  // (fma x, 1, y) -> (fadd x, y)
   // (fma x, -1, y) -> (fadd (fneg x), y)
   if (N1CFP) {
-    if (N1CFP->isExactlyValue(1.0))
-      // TODO: The FMA node should have flags that propagate to this node.
-      return DAG.getNode(ISD::FADD, DL, VT, N0, N2);
-
     if (N1CFP->isExactlyValue(-1.0) &&
         (!LegalOperations || TLI.isOperationLegal(ISD::FNEG, VT))) {
       SDValue RHSNeg = DAG.getNode(ISD::FNEG, DL, VT, N0);
       AddToWorklist(RHSNeg.getNode());
-      // TODO: The FMA node should have flags that propagate to this node.
-      return DAG.getNode(ISD::FADD, DL, VT, N2, RHSNeg);
+      return DAG.getNode(ISD::FADD, DL, VT, N2, RHSNeg, Flags);
     }
 
     // fma (fneg x), K, y -> fma x -K, y
     if (N0.getOpcode() == ISD::FNEG &&
         (TLI.isOperationLegal(ISD::ConstantFP, VT) ||
          (N1.hasOneUse() && !TLI.isFPImmLegal(N1CFP->getValueAPF(), VT,
                                               ForCodeSize)))) {
       return DAG.getNode(ISD::FMA, DL, VT, N0.getOperand(0),

llvm/test/CodeGen/PowerPC/fma-combine.ll

; CHECK-NEXT:    blr
   %add = fadd reassoc nsz double %x, 1.0
   %fma1 = call reassoc nsz double @llvm.fma.f64(double %fma, double %y, double %add)
   ret double %fma1
 }
 
 define double @fma_flag_propagation(double %a) {
 ; CHECK-FAST-LABEL: fma_flag_propagation:
 ; CHECK-FAST:       # %bb.0: # %entry
-; CHECK-FAST-NEXT:    xssubdp 1, 1, 1
+; CHECK-FAST-NEXT:    xxlxor 1, 1, 1
 ; CHECK-FAST-NEXT:    blr
 ;
 ; CHECK-FAST-NOVSX-LABEL: fma_flag_propagation:
 ; CHECK-FAST-NOVSX:       # %bb.0: # %entry
-; CHECK-FAST-NOVSX-NEXT:    fsub 1, 1, 1
+; CHECK-FAST-NOVSX-NEXT:    addis 3, 2, .LCPI6_0@toc@ha
+; CHECK-FAST-NOVSX-NEXT:    lfs 1, .LCPI6_0@toc@l(3)
 ; CHECK-FAST-NOVSX-NEXT:    blr
 ;
 ; CHECK-LABEL: fma_flag_propagation:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    xssubdp 1, 1, 1
+; CHECK-NEXT:    xxlxor 1, 1, 1
 ; CHECK-NEXT:    blr
 entry:
   %0 = fneg double %a
   %1 = call reassoc nnan double @llvm.fma.f64(double %0, double 1.0, double %a)
   ret double %1
 }
 
 declare double @llvm.fma.f64(double, double, double) nounwind readnone