[Bug target/53511] New: SH Target: Add support for fma patterns

[Bug target/53511] New: SH Target: Add support for fma patterns

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

             Bug #: 53511
           Summary: SH Target: Add support for fma patterns
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: olegendo@gcc.gnu.org
            Target: sh*-*-*


As Kaz already mentioned in PR 51340 comment #3 it should be OK to use SH's
FMAC insn to implement the new fma patterns.

[Bug target/53511] SH Target: Add support for fma patterns

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-06-03
         AssignedTo|unassigned at gcc dot       |olegendo at gcc dot gnu.org
                   |gnu.org                     |
     Ever Confirmed|0                           |1

[Bug target/53511] SH Target: Add support for fma patterns

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

--- Comment #1 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-06-03 18:01:31 UTC ---
Created attachment 27547
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27547
Propo

[Bug target/53511] SH Target: Add support for fma patterns

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kkojima at gcc dot gnu.org

--- Comment #2 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-06-03 18:29:32 UTC ---
The proposed patch adds the fmasf4 pattern and seems to be working OK (not
fully tested yet).

However, there are some side effects.
Because the option '-ffp-contract=' is set to 'fast' by default, the middle-end
will automatically convert expressions such as 'a * b + c' to fma patterns,
even without setting '-funsafe-math-optimizations'.  To disable this one has to
set '-ffp-contract=on' (see also
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48823#c3 ).

I'm afraid that if the fmasf4 pattern is always enabled, there could be some
issues with expected default behavior (as mentioned in
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51340#c1 ).
On the other hand, if the fmasf4 pattern is not always enabled, the std C
'fmaf' function can't be expanded to the 'fmac' insn.

Also, according to the discussion in PR 37845, it seems that the default
setting should leave the fma patterns enabled.

Effectively, the '*macsf3' / 'mac_media' patterns and the -mfused-madd /
-mno-fused-madd replicate middle-end functionality which is given by the 'fma'
patterns and the '-ffp-contract=' option.
Thus I'd like to propose to remove the '*macsf3' / 'mac_media' patterns and
deprecate the -mfused-madd / -mno-fused-madd options.

Kaz, what do you think?

[Bug target/53511] SH Target: Add support for fma patterns

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

--- Comment #3 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-06-03 22:48:17 UTC ---
(In reply to comment #2)
It would be better to ask these generic issues to the experts
on the gcc list, I think.  About the SH specific one,

> Effectively, the '*macsf3' / 'mac_media' patterns and the -mfused-madd /
> -mno-fused-madd replicate middle-end functionality which is given by the 'fma'
> patterns and the '-ffp-contract=' option.
> Thus I'd like to propose to remove the '*macsf3' / 'mac_media' patterns and
> deprecate the -mfused-madd / -mno-fused-madd options.

Sounds very plausible.

[Bug target/53511] SH Target: Add support for fma patterns

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27547|0                           |1
        is obsolete|                            |

--- Comment #4 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-06-05 02:41:54 UTC ---
Created attachment 27558
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27558
Proposed patch

This patch adds the FMA patterns and aligns the SH target with the -mfused-madd
/ -mno-fused-madd option handling in other targets.

Kaz, if you have some time and a SH64 test env at hand, could you please check
the patch on SH64?
I had to touch the __builtin_sh_media_FMAC_S mapping.  I think it should be OK,
since the number and semantics of the insn operands is the same as before.

[Bug target/53511] SH Target: Add support for fma patterns

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

--- Comment #5 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-06-05 04:47:22 UTC ---
(In reply to comment #4)
> This patch adds the FMA patterns and aligns the SH target with the -mfused-madd
> / -mno-fused-madd option handling in other targets.

Make -mfused-madd no-op instead to remove for the backward
compatibility.  Other than that, the patch looks fine to me.

> I had to touch the __builtin_sh_media_FMAC_S mapping.  I think it should be OK,
> since the number and semantics of the insn operands is the same as before.

It seems that __builtin_sh_media_FMAC_S is broken on trunk
in the first place, though I can test it only on sh64-elf
environment now.  Anyway the patch should OK in this regard.

[Bug target/53511] SH Target: Add support for fma patterns

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

--- Comment #6 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-06-05 09:21:49 UTC ---
> It seems that __builtin_sh_media_FMAC_S is broken on trunk
> in the first place, though I can test it only on sh64-elf
> environment now.  Anyway the patch should OK in this regard.

Looking into the __builtin_sh_media_FMAC_S implementation, it takes
2 floating arguments and one return float value.  It looks that
"a = __builtin_sh_media_FMAC_S (b, c)" was expected to work.  I guess
that this is broken in concept.  We can make it take 3 arguments but
I suspect that no one has used it at all.  It might be better to
simply remove this intrinsic because now your patch makes fma usable
naturally.

[Bug target/53511] SH Target: Add support for fma patterns

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

--- Comment #7 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-06-05 20:08:08 UTC ---
> (In reply to comment #4)
> 
> Make -mfused-madd no-op instead to remove for the backward
> compatibility.

Sorry, I don't quite follow.  According to my understanding we have
something like the following...

(A): New behavior (attachment 27558, using common gcc/config/fused-madd.opt)
(B): Old behavior
(C): New behavior, with -mfused-madd being a nop

                 |                  |  (A)     |   (B)    |   (C)
1st option       | 2nd option       | fma pat. | fmac pat.| fma pat. (new)
                 |                  | (new)    | (old)    | (mfused-madd = nop)
=================+==================+==========+==========+==================
<no option>      | -ffp-contract=   | 1        | 0        | 1
                 |  <no opt = fast> |          |          |
-----------------+------------------+----------+----------+------------------
<no option>      | -fpp-contract=off| 0        | 0        | 0
-----------------+------------------+----------+----------+------------------
-mfused-madd     | -ffp-contract=   | 1        | 1        | 1
                 |  <no opt = fast> |          |          |
-----------------+------------------+----------+----------+------------------
-mno-fused-madd  | -ffp-contract=   | 0        | 0        | 0
                 |  <no opt = fast> |          |          |
-----------------+------------------+----------+----------+------------------
-mfused-madd     | -fpp-contract=off| 0        | 1        | 0
-----------------+------------------+----------+----------+------------------
-mno-fused-madd  | -fpp-contract=off| 0        | 0        | 0
-----------------+------------------+----------+----------+------------------
-fpp-contract=off| -mfused-madd     | 1        | 1        | 0  (*)
-----------------+------------------+----------+----------+------------------
-fpp-contract=off| -mno-fused-madd  | 0        | 0        | 0
-----------------+------------------+----------+----------+------------------

My intention was to avoid having any special -mfused-madd handling in the SH
target code and to re-use the existing common gcc/config/fused-madd.opt.

Making -mfused-madd a no-op with special handling will only differ in the (*)
marked case.  Is this what you had in mind?

[Bug target/53511] SH Target: Add support for fma patterns

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

--- Comment #8 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-06-05 20:12:51 UTC ---
(In reply to comment #6)
> Looking into the __builtin_sh_media_FMAC_S implementation, it takes
> 2 floating arguments and one return float value.  It looks that
> "a = __builtin_sh_media_FMAC_S (b, c)" was expected to work.  I guess
> that this is broken in concept.  

I really wonder what the expected result should be in this case ;)

> We can make it take 3 arguments but
> I suspect that no one has used it at all.  It might be better to
> simply remove this intrinsic because now your patch makes fma usable
> naturally.

OK, I will just remove this built-in in the final patch.

[Bug target/53511] SH Target: Add support for fma patterns

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

--- Comment #9 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-06-05 22:35:28 UTC ---
(In reply to comment #7)
> Making -mfused-madd a no-op with special handling will only differ in the (*)
> marked case.  Is this what you had in mind?

Yes.  -mfused-madd is for a micro optimization after all.  It would
be tolerable even if it can't generate fmac insns in the case like (*).
OTOH deleting that option surprises the old programs which use
-mfused-madd.  I have no strong opinion, though.  On second thought,
-mfused-madd would be relatively rare in real use.

[Bug target/53511] SH Target: Add support for fma patterns

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

--- Comment #10 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-06-05 23:06:33 UTC ---
(In reply to comment #9)
> Yes.  -mfused-madd is for a micro optimization after all.  It would
> be tolerable even if it can't generate fmac insns in the case like (*).
> OTOH deleting that option surprises the old programs which use
> -mfused-madd.

I thought, that's what the common gcc/config/fused-madd.opt is for.  It does
not delete the old -mfused-madd option but issues a warning and sets
-fpp-contract=fast.  So older code that used -mfused-madd will continue to work
as before.
The big change will be for code that must not be compiled with FMA insns for
'a*b+c', because now GCC's behavior is to enable FMA by default.  Those older
programs will probably have no -mfused-madd option set and will have to be
patched to add the -fpp-contract=off option anyway.  I guess that this case
will be the minority.

Actually, adopting the -mfused-madd behavior from gcc/config/fused-madd.opt
would allow to add a default option -fpp-contract=off for certain compiler
builds (like -msoft-atomic on sh-linux) and then still allow programs to
override this by specifying -mfused-madd.

>  I have no strong opinion, though.  On second thought,
> -mfused-madd would be relatively rare in real use.

Yeah, probably.  I also don't care that much which option is on/off by default.
 I just wanted it to be aligned with the other targets and the overall target
independent GCC behavior, that's all :)

[Bug target/53511] SH Target: Add support for fma patterns

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

--- Comment #11 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-06-05 23:49:48 UTC ---
(In reply to comment #10)
> Yeah, probably.  I also don't care that much which option is on/off by default.
>  I just wanted it to be aligned with the other targets and the overall target
> independent GCC behavior, that's all :)

OK, please go ahead with the final patch as you like.

[Bug target/53511] SH Target: Add support for fma patterns

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

--- Comment #12 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-06-11 19:24:24 UTC ---
Author: olegendo
Date: Mon Jun 11 19:24:20 2012
New Revision: 188396

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=188396
Log:
    PR target/53511
    * config/sh/sh.md (fmasf4): New expander.
    (*macsf3): Rename to fmasf4_i.  Adapt to fma pattern.
    (mac_media): Rename to fmasf4_media.  Adapt to fma pattern.
    * config/sh/sh.opt (mfused-madd): Remove.
    * config/sh/sh.c (sh_option_override): Remove mfused-madd handling.
    (builtin_description bdesc): Remove __builtin_sh_media_FMAC_S.
    * config.gcc (sh[123456789lbe]*-*-* | sh-*-*): Add fused-madd.opt
    as extra options.
    * doc/invoke.texi (SH Options): Update mfused-madd and mno-fused-madd
    descriptions.

    PR target/53511
    * gcc.target/sh/pr53511-1.c: New.


Added:
    trunk/gcc/testsuite/gcc.target/sh/pr53511-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config.gcc
    trunk/gcc/config/sh/sh.c
    trunk/gcc/config/sh/sh.md
    trunk/gcc/config/sh/sh.opt
    trunk/gcc/doc/invoke.texi
    trunk/gcc/testsuite/ChangeLog

[Bug target/53511] SH Target: Add support for fma patterns

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

--- Comment #13 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-06-12 18:25:46 UTC ---
Author: olegendo
Date: Tue Jun 12 18:25:40 2012
New Revision: 188471

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=188471
Log:
    PR target/53511
    * gcc.target/sh/pr51340-1.c: Delete obsolete test case.
    * gcc.target/sh/pr51340-2.c: Likewise.
    * gcc.target/sh/pr51340-3.c: Likewise.


Removed:
    trunk/gcc/testsuite/gcc.target/sh/pr51340-1.c
    trunk/gcc/testsuite/gcc.target/sh/pr51340-2.c
    trunk/gcc/testsuite/gcc.target/sh/pr51340-3.c
Modified:
    trunk/gcc/testsuite/ChangeLog

[Bug target/53511] SH Target: Add support for fma patterns

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

--- Comment #14 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-07-23 22:54:13 UTC ---
Author: olegendo
Date: Mon Jul 23 22:54:06 2012
New Revision: 189796

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=189796
Log:
    PR target/53511
    * config/sh/sh.md (mulsf3_ie): Delete.
    (mulsf3_i4): Rename to mulsf3_i.
    (mulsf3): Emit mulsf3_i insn.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/sh/sh.md

[Bug target/53511] SH Target: Add support for fma patterns

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53511

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #15 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-07-23 23:00:36 UTC ---
Should be OK now.