[PATCH] D87037: [DAGCombiner] Propagate FMF flags in FMA folding

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

> However, besides this, SDNodeFlags are easily missed/passed wrongly. (D86871 <https://reviews.llvm.org/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?



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13191
   if (N1CFP) {
-    if (N1CFP->isExactlyValue(1.0))
-      // TODO: The FMA node should have flags that propagate to this node.
----------------
If this block is dead code, you can remove it independently of this patch.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13194
       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);
     }
----------------
Add a test (maybe with vector types) for this code path?


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

https://reviews.llvm.org/D87037