[Bug target/53511] SH Target: Add support for fma patterns
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=gccview=revrev=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
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.
[Bug target/53511] SH Target: Add support for fma patterns
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=gccview=revrev=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
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=gccview=revrev=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
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
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
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
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
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
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
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
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
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
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
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
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.
