* [patch,committed] Work around problem in gen-pass-instances.awk
@ 2016-10-26 9:54 Georg-Johann Lay
2016-10-26 10:48 ` Jakub Jelinek
0 siblings, 1 reply; 3+ messages in thread
From: Georg-Johann Lay @ 2016-10-26 9:54 UTC (permalink / raw)
To: gcc-patches
gen-pass-instances.awk is sensitive to the order in which directives are
written down, e.g. in target-pass.def: If a pass that runs first is added
first, then the last pass is skipped and not added to pass-instances.def.
Work around is to add the 2nd pass before adding the 1st pass...
http://gcc.gnu.org/r241547
No so obvious, but committed anyway...
Johann
gen-pass-instances.awk is sensitive to the order in which
passes are added; passes that appear later have to be added first.
PR target/71676
PR target/71678
* config/avr/avr-passes.def: Swap order of directives for
gen-pass-instances.awk.
Index: config/avr/avr-passes.def
===================================================================
--- config/avr/avr-passes.def (revision 241546)
+++ config/avr/avr-passes.def (revision 241547)
@@ -17,6 +17,19 @@
along with GCC; see the file COPYING3. If not see
<http://www.gnu.org/licenses/>. */
+/* FIXME: We have to add the last pass first, otherwise
+ gen-pass-instances.awk won't work as expected. */
+
+/* This avr-specific pass (re)computes insn notes, in particular REG_DEAD
+ notes which are used by `avr.c::reg_unused_after' and branch offset
+ computations. These notes must be correct, i.e. there must be no
+ dangling REG_DEAD notes; otherwise wrong code might result, cf. PR64331.
+
+ DF needs (correct) CFG, hence right before free_cfg is the last
+ opportunity to rectify notes. */
+
+INSERT_PASS_BEFORE (pass_free_cfg, 1, avr_pass_recompute_notes);
+
/* casesi uses a SImode switch index which is quite costly as most code will
work on HImode or QImode. The following pass runs right after .expand and
tries to fix such situations by operating on the original mode. This
@@ -27,13 +40,3 @@
insns withaout any insns in between. */
INSERT_PASS_AFTER (pass_expand, 1, avr_pass_casesi);
-
-/* This avr-specific pass (re)computes insn notes, in particular REG_DEAD
- notes which are used by `avr.c::reg_unused_after' and branch offset
- computations. These notes must be correct, i.e. there must be no
- dangling REG_DEAD notes; otherwise wrong code might result, cf. PR64331.
-
- DF needs (correct) CFG, hence right before free_cfg is the last
- opportunity to rectify notes. */
-
-INSERT_PASS_BEFORE (pass_free_cfg, 1, avr_pass_recompute_notes);
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [patch,committed] Work around problem in gen-pass-instances.awk 2016-10-26 9:54 [patch,committed] Work around problem in gen-pass-instances.awk Georg-Johann Lay @ 2016-10-26 10:48 ` Jakub Jelinek 2016-10-26 11:08 ` Richard Biener 0 siblings, 1 reply; 3+ messages in thread From: Jakub Jelinek @ 2016-10-26 10:48 UTC (permalink / raw) To: Richard Biener, Georg-Johann Lay; +Cc: gcc-patches On Wed, Oct 26, 2016 at 11:54:36AM +0200, Georg-Johann Lay wrote: > gen-pass-instances.awk is sensitive to the order in which directives are > written down, e.g. in target-pass.def: If a pass that runs first is added > first, then the last pass is skipped and not added to pass-instances.def. > > Work around is to add the 2nd pass before adding the 1st pass... > > http://gcc.gnu.org/r241547 > > No so obvious, but committed anyway... We shouldn't be working around bugs, but fixing them. Here is a fix (so far tested only with running for i in alpha avr i386 sparc aarch64; do awk -f ./gen-pass-instances.awk.jj passes.def config/$i/${i}-passes.def > /tmp/1 awk -f ./gen-pass-instances.awk passes.def config/$i/${i}-passes.def > /tmp/2 diff -up /tmp/1 /tmp/2 # and diff -upb done ), for avr for both avr-passes.def before your workaround and after it. Except for the desirable whitespace changes, the only change is in the pre-workaround avr-passes.def having the same output as after-workaround (except for the comments at the end of file). The fix for the avr issue is just the first hunk, the first part of the second hunk attempts to deal with NEXT_PASS ( avr_pass_casesi, 1); instead of the desirable NEXT_PASS (avr_pass_casesi, 1); (all arguments have the whitespace they have in *.def before them (after , or ( characters). So the first part of the second hunk strips away whitespace, so that one doesn't need to type INSERT_PASS_BEFORE (pass_free_cfg, 1,avr_pass_casesi); and the second part of the second hunk and third hunk deal with similar issue for the optional argument, we used to emit NEXT_PASS_WITH_ARG (pass_dominator, 1, false /* may_peel_loop_headers_p */); rather than NEXT_PASS_WITH_ARG (pass_dominator, 1, false /* may_peel_loop_headers_p */); because the space from NEXT_PASS (pass_dominator, false /* may_peel_loop_headers_p */); (in between , and false) is already there and we add another one. Ok for trunk if it passes bootstrap/regtest (though, as it doesn't change anything but whitespace on x86_64, it shouldn't make a difference)? 2016-10-26 Jakub Jelinek <jakub@redhat.com> * gen-pass-instances.awk (adjust_linenos): INcrement pass_lines[p] by increment rather than double it. (insert_remove_pass): Strip leading whitespace from args[3]. Don't emit a space before args[4]. (END): Don't emit a space before with_arg. --- gcc/gen-pass-instances.awk.jj 2016-10-10 10:41:32.000000000 +0200 +++ gcc/gen-pass-instances.awk 2016-10-26 12:30:20.637310242 +0200 @@ -90,7 +90,7 @@ function adjust_linenos(above, increment { for (p in pass_lines) if (pass_lines[p] >= above) - pass_lines[p] += pass_lines[p]; + pass_lines[p] += increment; if (increment > 0) for (i = lineno - 1; i >= above; i--) lines[i + increment] = lines[i]; @@ -100,16 +100,18 @@ function adjust_linenos(above, increment lineno += increment; } -function insert_remove_pass(line, fnname) +function insert_remove_pass(line, fnname, arg3) { parse_line($0, fnname); pass_name = args[1]; if (pass_name == "PASS") return 1; pass_num = args[2] + 0; - new_line = prefix "NEXT_PASS (" args[3]; + arg3 = args[3]; + sub(/^[ \t]*/, "", arg3); + new_line = prefix "NEXT_PASS (" arg3; if (args[4]) - new_line = new_line ", " args[4]; + new_line = new_line "," args[4]; new_line = new_line ")" postfix; if (!pass_lines[pass_name, pass_num]) { @@ -218,7 +220,7 @@ END { printf "NEXT_PASS"; printf " (%s, %s", pass_name, pass_num; if (with_arg) - printf ", %s", with_arg; + printf ",%s", with_arg; printf ")%s\n", postfix; } else Jakub ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch,committed] Work around problem in gen-pass-instances.awk 2016-10-26 10:48 ` Jakub Jelinek @ 2016-10-26 11:08 ` Richard Biener 0 siblings, 0 replies; 3+ messages in thread From: Richard Biener @ 2016-10-26 11:08 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Georg-Johann Lay, gcc-patches On Wed, 26 Oct 2016, Jakub Jelinek wrote: > On Wed, Oct 26, 2016 at 11:54:36AM +0200, Georg-Johann Lay wrote: > > gen-pass-instances.awk is sensitive to the order in which directives are > > written down, e.g. in target-pass.def: If a pass that runs first is added > > first, then the last pass is skipped and not added to pass-instances.def. > > > > Work around is to add the 2nd pass before adding the 1st pass... > > > > http://gcc.gnu.org/r241547 > > > > No so obvious, but committed anyway... > > We shouldn't be working around bugs, but fixing them. > > Here is a fix (so far tested only with running > for i in alpha avr i386 sparc aarch64; do > awk -f ./gen-pass-instances.awk.jj passes.def config/$i/${i}-passes.def > /tmp/1 > awk -f ./gen-pass-instances.awk passes.def config/$i/${i}-passes.def > /tmp/2 > diff -up /tmp/1 /tmp/2 # and diff -upb > done > ), for avr for both avr-passes.def before your workaround and after it. > Except for the desirable whitespace changes, the only change is in the > pre-workaround avr-passes.def having the same output as after-workaround > (except for the comments at the end of file). > > The fix for the avr issue is just the first hunk, the first part of the > second hunk attempts to deal with > NEXT_PASS ( avr_pass_casesi, 1); > instead of the desirable > NEXT_PASS (avr_pass_casesi, 1); > (all arguments have the whitespace they have in *.def before them (after , > or ( characters). So the first part of the second hunk strips away > whitespace, so that one doesn't need to type > INSERT_PASS_BEFORE (pass_free_cfg, 1,avr_pass_casesi); > and the second part of the second hunk and third hunk deal with similar > issue for the optional argument, we used to emit > NEXT_PASS_WITH_ARG (pass_dominator, 1, false /* may_peel_loop_headers_p */); > rather than > NEXT_PASS_WITH_ARG (pass_dominator, 1, false /* may_peel_loop_headers_p */); > because the space from > NEXT_PASS (pass_dominator, false /* may_peel_loop_headers_p */); > (in between , and false) is already there and we add another one. > > Ok for trunk if it passes bootstrap/regtest (though, as it doesn't change > anything but whitespace on x86_64, it shouldn't make a difference)? Ok. Richard. > 2016-10-26 Jakub Jelinek <jakub@redhat.com> > > * gen-pass-instances.awk (adjust_linenos): INcrement pass_lines[p] > by increment rather than double it. > (insert_remove_pass): Strip leading whitespace from args[3]. Don't > emit a space before args[4]. > (END): Don't emit a space before with_arg. > > --- gcc/gen-pass-instances.awk.jj 2016-10-10 10:41:32.000000000 +0200 > +++ gcc/gen-pass-instances.awk 2016-10-26 12:30:20.637310242 +0200 > @@ -90,7 +90,7 @@ function adjust_linenos(above, increment > { > for (p in pass_lines) > if (pass_lines[p] >= above) > - pass_lines[p] += pass_lines[p]; > + pass_lines[p] += increment; > if (increment > 0) > for (i = lineno - 1; i >= above; i--) > lines[i + increment] = lines[i]; > @@ -100,16 +100,18 @@ function adjust_linenos(above, increment > lineno += increment; > } > > -function insert_remove_pass(line, fnname) > +function insert_remove_pass(line, fnname, arg3) > { > parse_line($0, fnname); > pass_name = args[1]; > if (pass_name == "PASS") > return 1; > pass_num = args[2] + 0; > - new_line = prefix "NEXT_PASS (" args[3]; > + arg3 = args[3]; > + sub(/^[ \t]*/, "", arg3); > + new_line = prefix "NEXT_PASS (" arg3; > if (args[4]) > - new_line = new_line ", " args[4]; > + new_line = new_line "," args[4]; > new_line = new_line ")" postfix; > if (!pass_lines[pass_name, pass_num]) > { > @@ -218,7 +220,7 @@ END { > printf "NEXT_PASS"; > printf " (%s, %s", pass_name, pass_num; > if (with_arg) > - printf ", %s", with_arg; > + printf ",%s", with_arg; > printf ")%s\n", postfix; > } > else > > > Jakub > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-10-26 11:08 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-26 9:54 [patch,committed] Work around problem in gen-pass-instances.awk Georg-Johann Lay 2016-10-26 10:48 ` Jakub Jelinek 2016-10-26 11:08 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).