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.
Created attachment 27547 [details] Proposed patch
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?
(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.
Created attachment 27558 [details] 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.
(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.
> 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.
> (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 [details], 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?
(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.
(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.
(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 :)
(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.
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
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
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
Should be OK now.