[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 Andrew Pinski changed: What|Removed |Added Resolution|--- |FIXED Status|NEW |RESOLVED --- Comment #13 from Andrew Pinski --- .
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 --- Comment #12 from Hongyu Wang --- Fixed for GCC 13. Sorry for introducing this.
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 --- Comment #11 from CVS Commits --- The master branch has been updated by Hongyu Wang : https://gcc.gnu.org/g:8caf155a3d6e23e47bf55068ad23c23d4655a054 commit r13-4272-g8caf155a3d6e23e47bf55068ad23c23d4655a054 Author: Hongyu Wang Date: Sat Nov 19 09:38:00 2022 +0800 i386: Only enable small loop unrolling in backend [PR 107692] Followed by the discussion in pr107692, -munroll-only-small-loops Does not turns on/off -funroll-loops, and current check in pass_rtl_unroll_loops::gate would cause -fno-unroll-loops do not take effect. Revert the change about targetm.loop_unroll_adjust and apply the backend option change to strictly follow the rule that -funroll-loops takes full control of loop unrolling, and munroll-only-small-loops just change its behavior to unroll small size loops. gcc/ChangeLog: PR target/107692 * common/config/i386/i386-common.cc (ix86_optimization_table): Enable loop unroll O2, disable -fweb and -frename-registers by default. * config/i386/i386-options.cc (ix86_override_options_after_change): Disable small loop unroll when funroll-loops enabled, reset cunroll_grow_size when it is not explicitly enabled. (ix86_option_override_internal): Call ix86_override_options_after_change instead of calling ix86_recompute_optlev_based_flags and ix86_default_align separately. * config/i386/i386.cc (ix86_loop_unroll_adjust): Adjust unroll factor if -munroll-only-small-loops enabled. * loop-init.cc (pass_rtl_unroll_loops::gate): Do not enable loop unrolling for -O2-speed. (pass_rtl_unroll_loops::execute): Rmove targetm.loop_unroll_adjust check. gcc/testsuite/ChangeLog: PR target/107692 * gcc.dg/guality/loop-1.c: Remove additional option for ia32. * gcc.target/i386/pr86270.c: Add -fno-unroll-loops. * gcc.target/i386/pr93002.c: Likewise.
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692
--- Comment #10 from Segher Boessenkool ---
(In reply to Hongyu Wang from comment #9)
> The difference is, -mno-unroll-only-small-loops -O2 would cause
> rtl-loop-unroll takeing effect,
No. -m{no-,}unroll-only-small-loops does not enable or disable loop unrolling
at all. The only thing it does is modify which loops are candidate to be
unrolled.
> I think the intension of -munroll-only-small-loops is to just adjust
> rtl-loop-unrolling and do not touch middle-end unroll/cunroll.
It modifies the behaviour of -funroll-loops. It doesn't do anythyng else.
Anything that wants to see if unrolling is active can just look if
flag_unroll_loops is set. The sane and simple thing.
> But I think
> your point is also reasonable. Maybe we can split the flag_unroll_loops to
> tree and rtl seperately?
Users do not care if something is done on Gimple or on RTL. The command line
flags are for users. They work fine as-is.
> Anyway I will propose a patch and re-discuss with maintainers later. Thanks!
Please fix this regression asap. It is a P1, and we are in stage 3 already.
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 --- Comment #9 from Hongyu Wang --- (In reply to Segher Boessenkool from comment #8) > (In reply to Jiu Fu Guo from comment #5) > > > -munroll-only-small-loops does not turn on or off -funroll-loops, and it > > > should not, so that it does what it says, if nothing else. > > > > Yes, and -funroll-loops would win over -munroll-only-small-loops > > -funroll-loops is the only thing that enables loop unrolling. > -munroll-only-small-loops, like the name says, says to only unroll small > loops, > and no others. It is not something at the same level as -funroll-loops, that > would be insanity: other code likes to see if the user requested loops to be > unrolled as well! I can understand the logic, my initial patch https://gcc.gnu.org/pipermail/gcc-patches/2022-October/604345.html is something similar to rs6000 and x86 only. The difference is, -mno-unroll-only-small-loops -O2 would cause rtl-loop-unroll takeing effect, and cunroll will also work if we follow the rs6000 change. We do not really want these so the patch becomes ugly as said :( I think the intension of -munroll-only-small-loops is to just adjust rtl-loop-unrolling and do not touch middle-end unroll/cunroll. But I think your point is also reasonable. Maybe we can split the flag_unroll_loops to tree and rtl seperately? Anyway I will propose a patch and re-discuss with maintainers later. Thanks!
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 Segher Boessenkool changed: What|Removed |Added Status|UNCONFIRMED |NEW Ever confirmed|0 |1 Last reconfirmed||2022-11-18
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 --- Comment #8 from Segher Boessenkool --- (In reply to Jiu Fu Guo from comment #5) > > -munroll-only-small-loops does not turn on or off -funroll-loops, and it > > should not, so that it does what it says, if nothing else. > > Yes, and -funroll-loops would win over -munroll-only-small-loops -funroll-loops is the only thing that enables loop unrolling. -munroll-only-small-loops, like the name says, says to only unroll small loops, and no others. It is not something at the same level as -funroll-loops, that would be insanity: other code likes to see if the user requested loops to be unrolled as well!
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 --- Comment #7 from Jiu Fu Guo --- (In reply to Hongyu Wang from comment #6) > (In reply to Jiu Fu Guo from comment #4) > cut... > > Yes, I've already posted the patch at > https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606478.html One minor finding: Like -munroll-only-small-loops on other targets(e.g. rs6000, r13-3950 intends to enable unrolling and uses this option to control the unroll factor according to the loop size. Compare with the previous logic(e.g. for rs6000), the new logic will cause: -fno-unroll-loops may be unable to prevent rtl_unroll_loops from running, but loop_unroll_adjust will return 1 to prevent the loop to be unrolled. So, there may be side-effects on "slight compiling time" and "dump-file may be generated with the message of failing to unroll".
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 --- Comment #6 from Hongyu Wang --- (In reply to Jiu Fu Guo from comment #4) > (In reply to Hongyu Wang from comment #2) > > Created attachment 53897 [details] > > A patch > > > > Sorry for introducing these fails. Here is the patch. > > > > I've tested the patch with cross-compler and all the fails disappeared, but > > I don't have a powerpc to do full bootstrap & regtest (I'm still applying > > for gcc farm account). > > > > I'll send out the patch after I can access gcc farm for a power machine, or > > hopefully someone can help testing the patch. > > > > I suppose s390 has similar issue and I will update that accordingly. > Hi, > > One small comment, for code "if (!(flag_unroll_loops || > flag_unroll_all_loops))" > we may need to add one more condition "|| loop->unroll", like what does in > r13-3950 for i386.cc. Otherwise, unroll pragma may be affected. Yes, I've already posted the patch at https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606478.html
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 --- Comment #5 from Jiu Fu Guo --- > -munroll-only-small-loops does not turn on or off -funroll-loops, and it > should not, so that it does what it says, if nothing else. Yes, and -funroll-loops would win over -munroll-only-small-loops
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 Jiu Fu Guo changed: What|Removed |Added CC||guojiufu at gcc dot gnu.org --- Comment #4 from Jiu Fu Guo --- (In reply to Hongyu Wang from comment #2) > Created attachment 53897 [details] > A patch > > Sorry for introducing these fails. Here is the patch. > > I've tested the patch with cross-compler and all the fails disappeared, but > I don't have a powerpc to do full bootstrap & regtest (I'm still applying > for gcc farm account). > > I'll send out the patch after I can access gcc farm for a power machine, or > hopefully someone can help testing the patch. > > I suppose s390 has similar issue and I will update that accordingly. Hi, One small comment, for code "if (!(flag_unroll_loops || flag_unroll_all_loops))" we may need to add one more condition "|| loop->unroll", like what does in r13-3950 for i386.cc. Otherwise, unroll pragma may be affected.
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 Richard Biener changed: What|Removed |Added Priority|P3 |P1
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 --- Comment #3 from Segher Boessenkool --- Hi! (In reply to Hongyu Wang from comment #2) > I've tested the patch with cross-compler and all the fails disappeared, but > I don't have a powerpc to do full bootstrap & regtest (I'm still applying > for gcc farm account). [ I approved that account ] -munroll-only-small-loops does not turn on or off -funroll-loops, and it should not, so that it does what it says, if nothing else. We do not want -frename-registers either (at -O2 at least), it hurts performance.
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 --- Comment #2 from Hongyu Wang --- Created attachment 53897 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53897=edit A patch Sorry for introducing these fails. Here is the patch. I've tested the patch with cross-compler and all the fails disappeared, but I don't have a powerpc to do full bootstrap & regtest (I'm still applying for gcc farm account). I'll send out the patch after I can access gcc farm for a power machine, or hopefully someone can help testing the patch. I suppose s390 has similar issue and I will update that accordingly.
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 Andrew Pinski changed: What|Removed |Added Target Milestone|--- |13.0
[Bug target/107692] [13 regression] r13-3950-g071e428c24ee8c breaks many test cases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107692 --- Comment #1 from Andrew Pinski --- Some of these testcases might need -mno-unroll-only-small-loops now.
