[PATCH] D28675: [DAGCombine] require UnsafeFPMath for re-association of addition

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

@arsenm, unfortunately I can't add flag-based tests, since as @hfinkel points out, you can't actually get the flag on an FMA or FMAD in the DAG. @hfinkel, the idea was to put the check in already anyway, since by the discussion with @spatel, the plan is to add FMF for all nodes eventually. That seems reasonable to me.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8128
     // fold (fadd (fma x, y, (fmul u, v)), z) -> (fma x, y (fma u, v, z))
+    // FIXME The UnsafeAlgebra flag should be propagated to FMA/FMAD, but FMF
+    // are currently only supported on binary nodes.
----------------
arsenm wrote:
> : after FIXME (same for the other places)
Done.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8134
+        (Options.UnsafeFPMath || (N->getFlags()->hasUnsafeAlgebra() &&
+         N0.getNode()->getFlags()->hasUnsafeAlgebra()))) {
       return DAG.getNode(PreferredFusedOpcode, SL, VT,
----------------
hfinkel wrote:
> arsenm wrote:
> > I don't think you need the getNode here (and same for the rest
> Why are you checking N0->getFlags()->hasUnsafeAlgebra() if N0 is a FMA and the comment says they don't support FMFs?
> 
I double-checked, SDValue doesn't have getFlags().


https://reviews.llvm.org/D28675