[PATCH] D45710: Fast Math Flag mapping into SDNode

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec 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())) &&
----------------
mcberg2017 wrote:
> 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.
Thanks. I think we are fine then.


https://reviews.llvm.org/D45710