[PATCH] D72675: Fix -ffast-math/-ffp-contract interaction

[Warren Ristow via Phabricator via llvm-commits]

wristow marked an inline comment as done.
wristow added inline comments.


================
Comment at: llvm/test/CodeGen/PowerPC/fmf-propagation.ll:201-203
 ; fma(X, 7.0, X * 42.0) --> X * 49.0
-; This is the minimum FMF needed for this transform - the FMA allows reassociation.
+; This is the minimum FMF needed for this transform - the 'contract' allows the needed reassociation.
+
----------------
spatel wrote:
> I was ok with the reasoning up to here, but this example lost me.
> Why does 'contract' alone allow splitting an fma?
> Is 'contract' relevant on anything besides fadd (with an fmul operand)?
I have to admit I'm handwaving here.  I don't really understand why 'contract' alone allows this simpliciation:
`fma(X, 7.0, X * 42.0) --> X * 49.0`

to happen.  But either that's correct, or it's a separate bug (and I waved my hands about explaining more detail).

The short story is that even without my proposed change, having //only// 'contract' on the FMA intrinsic results in this being simplified to `X * 49` (and also having //only// 'reassoc' did).  The original comment:

`; This is the minimum FMF needed for this transform - the FMA allows reassociation.`

is incomplete/misleading.  A more complete comment (relevant before my change) would have been:

`; This is the minimum FMF needed for this transform - either 'reassoc' or 'contract' applied to the FMA intrinsic allows reassociation.`

And with my change, the result is that 'reassoc' is no longer relevant.  But since I don't really understand why it should be simplified with only `contract`, I should put a TODO comment in.  That is, to me it seems like both 'contract' //and// 'reassoc' should be needed for this simplification to happen (whereas previously it worked with //either// of them individually).  So maybe change this comment to:

    ; This is the minimum FMF needed for this transform - applying 'contract' to the FMA intrinsic allows reassociation.
    ; TODO: It seems 'contract' and 'reassoc' combined should be needed for this transform.  Why does it work with only 'contract'?


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

https://reviews.llvm.org/D72675