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

[Warren Ristow via Phabricator via cfe-commits]

wristow added a comment.

> One commit for the clang changes should be ok; it's a very small diff. But 
> I'm still not sure if the driver change induces frontend diffs that we should 
> make visible via tests.

The only thing I can think of is that it changes whether/when `__FAST_MATH__` 
is defined.  But that'll be indirectly tested, via the updated tests in 
"Driver/fast-math.c", which will verify that "-ffast-math" is passed 
appropriately (and the `__FAST_MATH__` dependency on "-ffast-math" is already 
tested in "Preprocessor/predefined-macros.c").

In addition to adding the second pair of driver tests you suggested for 
`-ffp-contract=on`, I'll also add another pair for `-ffp-contract=fast` to 
verify that "-ffast-math" is passed in that case (no change in behavior for 
that pair, but just confirming it continues to work correctly).


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

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

[Sanjay Patel via Phabricator via cfe-commits]

spatel added a comment.

In D72675#1824659 , @wristow wrote:

> This all sounds good to me.
>
> So to make sure we're all on the same page, my understanding is that the plan 
> forward is:
>
> 1. Make the Clang change first (including adding another pair of tests for 
> `-ffp-contract=on`).
> 2. Update the LLVM tests illustrating the current baseline state.
> 3. Make the LLVM change shown here, and update the tests with the fixes.
> 4. Bring in the DAG combiner work that Michael has done internally at Apple.
>
>   Points 1, 2, and 3 are essentially the points in the patch posted here, so 
> I'll do that.  And of course Michael will then take on point 4.
>
>   Is that the plan?  If yes, I'll transition this item to just be the Clang 
> pieces, and I'll spin off a new one to do the LLVM portion of what is posted 
> here.


SGTM

> Sanjay, regarding:
> 
>> But it would be better to have all of the baseline tests in place, so we 
>> know where we stand currently.
> 
> I'm interpreting that as applying to the LLVM tests.  That is, the Clang 
> change, along with the updated tests, can be in one commit, rather than first 
> updating the Clang driver tests with the current state.  If you'd prefer two 
> commits on the Clang side as well, let me know.

One commit for the clang changes should be ok; it's a very small diff. But I'm 
still not sure if the driver change induces frontend diffs that we should make 
visible via tests.


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

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

[Warren Ristow via Phabricator via cfe-commits]

wristow added a comment.

This all sounds good to me.

So to make sure we're all on the same page, my understanding is that the plan 
forward is:

1. Make the Clang change first (including adding another pair of tests for 
`-ffp-contract=on`).
2. Update the LLVM tests illustrating the current baseline state.
3. Make the LLVM change shown here, and update the tests with the fixes.
4. Bring in the DAG combiner work that Michael has done internally at Apple.

Points 1, 2, and 3 are essentially the points in the patch posted here, so I'll 
do that.  And of course Michael will then take on point 4.

Is that the plan?  If yes, I'll transition this item to just be the Clang 
pieces, and I'll spin off a new one to do the LLVM portion of what is posted 
here.

Sanjay, regarding:

> But it would be better to have all of the baseline tests in place, so we know 
> where we stand currently.

I'm interpreting that as applying to the LLVM tests.  That is, the Clang 
change, along with the updated tests, can be in one commit, rather than first 
updating the Clang driver tests with the current state.  If you'd prefer two 
commits on the Clang side as well, let me know.


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

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

[Michael Berg via Phabricator via cfe-commits]

mcberg2017 added a comment.

I should do the other DAG combiner fma changes after this is wrapped up.


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

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

[Sanjay Patel via Phabricator via cfe-commits]

spatel added a comment.

In D72675#1823227 , @mcberg2017 wrote:

> We crossed that bridge internally at Apple a while ago, meaning I have some 
> code debt for cleaning up open source for fma formation that uses contract 
> and reassoc differently than we do today, both together and separately, case 
> by case.


Ah, great. I don't have a preference about the order of patches (ie, is it 
better to make this change first or do the backend cleanup first), so whatever 
works better.

But it would be better to have all of the baseline tests in place, so we know 
where we stand currently. So I'd prefer to have the PPC and x86 tests 
pre-committed to master (change test comments as needed to match current/future 
reality).

We could also decouple the clang part of this patch from the backend change 
IIUC. But do we need another change (or at least tests) to show how the driver 
difference translates in the clang front-end to LLVM FMF and/or function 
attributes?




Comment at: clang/test/Driver/fast-math.c:183-184
 //
+// -ffp-contract=off must disable the fast-math umbrella, and the 
unsafe-fp-math
+// umbrella.
+// RUN: %clang -### -ffast-math -ffp-contract=off -c %s 2>&1 \

Do we need another pair of tests for -ffp-contract=on?


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

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

[Michael Berg via Phabricator via cfe-commits]

mcberg2017 added a comment.

We crossed that bridge internally at Apple a while ago, meaning I have some 
code debt for cleaning up open source for fma formation that uses contract and 
reassoc differently than we do today, both together and separately, case by 
case.


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

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

[Warren Ristow via Phabricator via cfe-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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sanjay Patel via Phabricator via cfe-commits]

spatel 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.
+

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)?


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

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

[Warren Ristow via Phabricator via cfe-commits]

wristow updated this revision to Diff 238143.
wristow retitled this revision from "ix -ffast-math/-ffp-contract interaction" 
to "Fix -ffast-math/-ffp-contract interaction".
wristow added a comment.

Addressed comments from @hfinkel .


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

https://reviews.llvm.org/D72675

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/fast-math.c
  llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
  llvm/test/CodeGen/PowerPC/fmf-propagation.ll
  llvm/test/CodeGen/X86/fp-contract.ll

Index: llvm/test/CodeGen/X86/fp-contract.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/fp-contract.ll
@@ -0,0 +1,204 @@
+; Tests for -ffp-contract/-ffast-math interaction.
+; Specifically, -ffp-contract=off must suppress the use of FMA.
+
+; RUN: llc < %s -mcpu=haswell | FileCheck %s --check-prefix=FMA
+
+; Scalar versions:
+
+define float @MulAddPlain(float %a, float %b, float %c) {
+; FMA-LABEL: MulAddPlain:
+; FMA:   vmulss
+; FMA-NEXT:  vaddss
+; FMA-NEXT:  ret
+  %mul = fmul float %a, %b
+  %add = fadd float %mul, %c
+  ret float %add
+}
+
+define float @MulAddFast(float %a, float %b, float %c) {
+; FMA-LABEL: MulAddFast:
+; FMA:   vfmadd213ss
+; FMA-NEXT:  ret
+  %mul = fmul fast float %a, %b
+  %add = fadd fast float %mul, %c
+  ret float %add
+}
+
+define float @MulAddContract(float %a, float %b, float %c) {
+; FMA-LABEL: MulAddContract:
+; FMA:   vfmadd213ss
+; FMA-NEXT:  ret
+  %mul = fmul contract float %a, %b
+  %add = fadd contract float %mul, %c
+  ret float %add
+}
+
+; Enabling all the fast-math-flags except 'contract' does not enable fused operations.
+define float @MulAddFastNoContract(float %a, float %b, float %c) {
+; FMA-LABEL: MulAddFastNoContract:
+; FMA:   vmulss
+; FMA-NEXT:  vaddss
+; FMA-NEXT:  ret
+  %mul = fmul nnan ninf nsz arcp afn reassoc float %a, %b
+  %add = fadd nnan ninf nsz arcp afn reassoc float %mul, %c
+  ret float %add
+}
+
+define float @MulAddReassoc(float %a, float %b, float %c) {
+; FMA-LABEL: MulAddReassoc:
+; FMA:   vmulss
+; FMA-NEXT:  vaddss
+; FMA-NEXT:  ret
+  %mul = fmul reassoc float %a, %b
+  %add = fadd reassoc float %mul, %c
+  ret float %add
+}
+
+define float @MulSubPlain(float %a, float %b, float %c) {
+; FMA-LABEL: MulSubPlain:
+; FMA:   vmulss
+; FMA-NEXT:  vsubss
+; FMA-NEXT:  ret
+  %mul = fmul float %a, %b
+  %sub = fsub float %mul, %c
+  ret float %sub
+}
+
+define float @MulSubFast(float %a, float %b, float %c) {
+; FMA-LABEL: MulSubFast:
+; FMA:   vfmsub213ss
+; FMA-NEXT:  ret
+  %mul = fmul fast float %a, %b
+  %sub = fsub fast float %mul, %c
+  ret float %sub
+}
+
+define float @MulSubContract(float %a, float %b, float %c) {
+; FMA-LABEL: MulSubContract:
+; FMA:   vfmsub213ss
+; FMA-NEXT:  ret
+  %mul = fmul contract float %a, %b
+  %sub = fsub contract float %mul, %c
+  ret float %sub
+}
+
+; Enabling all the fast-math-flags except 'contract' does not enable fused operations.
+define float @MulSubFastNoContract(float %a, float %b, float %c) {
+; FMA-LABEL: MulSubFastNoContract:
+; FMA:   vmulss
+; FMA-NEXT:  vsubss
+; FMA-NEXT:  ret
+  %mul = fmul nnan ninf nsz arcp afn reassoc float %a, %b
+  %sub = fsub nnan ninf nsz arcp afn reassoc float %mul, %c
+  ret float %sub
+}
+
+define float @MulSubReassoc(float %a, float %b, float %c) {
+; FMA-LABEL: MulSubReassoc:
+; FMA:   vmulss
+; FMA-NEXT:  vsubss
+; FMA-NEXT:  ret
+  %mul = fmul reassoc float %a, %b
+  %sub = fsub reassoc float %mul, %c
+  ret float %sub
+}
+
+; Vector versions:
+
+define <4 x float> @VecMulAddPlain(<4 x float> %a, <4 x float> %b, <4 x float> %c) {
+; FMA-LABEL: VecMulAddPlain:
+; FMA:   vmulps
+; FMA-NEXT:  vaddps
+; FMA-NEXT:  ret
+  %mul = fmul <4 x float> %a, %b
+  %add = fadd <4 x float> %mul, %c
+  ret <4 x float> %add
+}
+
+define <4 x float> @VecMulAddFast(<4 x float> %a, <4 x float> %b, <4 x float> %c) {
+; FMA-LABEL: VecMulAddFast:
+; FMA:   vfmadd213ps
+; FMA-NEXT:  ret
+  %mul = fmul fast <4 x float> %a, %b
+  %add = fadd fast <4 x float> %mul, %c
+  ret <4 x float> %add
+}
+
+define <4 x float> @VecMulAddContract(<4 x float> %a, <4 x float> %b, <4 x float> %c) {
+; FMA-LABEL: VecMulAddContract:
+; FMA:   vfmadd213ps
+; FMA-NEXT:  ret
+  %mul = fmul contract <4 x float> %a, %b
+  %add = fadd contract <4 x float> %mul, %c
+  ret <4 x float> %add
+}
+
+; Enabling all the fast-math-flags except 'contract' does not enable fused operations.
+define <4 x float> @VecMulAddFastNoContract(<4 x float> %a, <4 x float> %b, <4 x float> %c) {
+; FMA-LABEL: VecMulAddFastNoContract:
+; FMA:   vmulps
+; FMA-NEXT:  vaddps
+; FMA-NEXT:  ret
+  %mul = fmul nnan ninf nsz arcp afn reassoc <4 x float> %a, %b
+  %add = fadd nnan ninf nsz arcp afn reassoc <4 x float> %mul, %c
+  ret <4 x float> %add
+}
+
+define <4 x float> @VecMulAddReassoc(<4 x float> %a, <4 x float> %b, <4 x float> %c) {
+; FMA-LABEL: VecMulAddReassoc: