[PATCH] D45710: Fast Math Flag mapping into SDNode

[Michael Berg via Phabricator via llvm-commits]

mcberg2017 added inline comments.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:6700
   if ((Options.AllowFPOpFusion == FPOpFusion::Fast || Options.UnsafeFPMath ||
-       (N0->getFlags().hasUnsafeAlgebra() &&
-        N1->getFlags().hasUnsafeAlgebra())) &&
+       (N0->getFlags().hasAllowReassociation() &&
+        N1->getFlags().hasAllowReassociation())) &&
----------------
rampitec wrote:
> spatel wrote:
> > mcberg2017 wrote:
> > > rampitec wrote:
> > > > mcberg2017 wrote:
> > > > > rampitec wrote:
> > > > > > Production of a fused fma opcode is not tied to reassociation. The question here is legality of intermediate result rounding.
> > > > > Stanislav, what change if any do you request then for this code?
> > > > I would say AllowContract can be added here, but reassociation is not a sufficient or required condition.
> > > Ok, will do.
> > So this is an existing bug in the code? It shouldn't be checking hasUnsafeAlgebra (or Options.UnsafeFPMath). Can we add a test for that?
> I do not think it is a bug, this is legal to do with unsafe algebra.
> At the end of the day I think if you remove reassociation from the condition and add AllowContract check it should then boil down to the question if existing lit tests will pass.
With the change to hasAllowContract, existing lit tests all pass.


https://reviews.llvm.org/D45710