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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
