| Message ID | 20200727074608.12315-1-hujiangping@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] driver: fix a problem with implementation of -falign-foo=0 [PR96247] | expand |
Tested on x86_64. > -----Original Message----- > From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of Hu > Jiangping > Sent: Monday, July 27, 2020 3:46 PM > To: gcc-patches@gcc.gnu.org; richard.sandiford@arm.com; > segher@kernel.crashing.org > Subject: [PATCH v3] driver: fix a problem with implementation of -falign-foo=0 > [PR96247] > > Hi! > > This patch makes the -falign-foo=0 work as described in the > documentation. Thanks for all the suggestions, Richard and Segher! > > v3: make change more readable and self-consistent > v2: at a high level handles -falign-foo=0 like -falign-foo > > Regards! > Hujp > > --- > gcc/opts.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/gcc/opts.c b/gcc/opts.c > index 499eb900643..dec5ba6d2be 100644 > --- a/gcc/opts.c > +++ b/gcc/opts.c > @@ -2007,10 +2007,20 @@ parse_and_check_align_values (const char *flag, > location LOC. */ > > static void > -check_alignment_argument (location_t loc, const char *flag, const char > *name) > +check_alignment_argument (location_t loc, > + const char *flag, > + const char *name, > + int *opt_flag, > + const char **opt_str) > { > auto_vec<unsigned> align_result; > parse_and_check_align_values (flag, name, align_result, true, loc); > + > + if (align_result.length() >= 1 && align_result[0] == 0) > + { > + *opt_flag = 1; > + *opt_str = NULL; > + } > } > > /* Print help when OPT__help_ is set. */ > @@ -2785,19 +2795,23 @@ common_handle_option (struct gcc_options > *opts, > break; > > case OPT_falign_loops_: > - check_alignment_argument (loc, arg, "loops"); > + check_alignment_argument (loc, arg, "loops", > + &opts->x_flag_align_loops, &opts->x_str_align_loops); > break; > > case OPT_falign_jumps_: > - check_alignment_argument (loc, arg, "jumps"); > + check_alignment_argument (loc, arg, "jumps", > + &opts->x_flag_align_jumps, &opts->x_str_align_jumps); > break; > > case OPT_falign_labels_: > - check_alignment_argument (loc, arg, "labels"); > + check_alignment_argument (loc, arg, "labels", > + &opts->x_flag_align_labels, &opts->x_str_align_labels); > break; > > case OPT_falign_functions_: > - check_alignment_argument (loc, arg, "functions"); > + check_alignment_argument (loc, arg, "functions", > + &opts->x_flag_align_functions, &opts->x_str_align_functions); > break; > > case OPT_ftabstop_: > -- > 2.17.1 > >
Hu Jiangping <hujiangping@cn.fujitsu.com> writes: > Hi! > > This patch makes the -falign-foo=0 work as described in the > documentation. Thanks for all the suggestions, Richard and Segher! > > v3: make change more readable and self-consistent > v2: at a high level handles -falign-foo=0 like -falign-foo Thanks, this is OK with some minor nits for coding conventions. Could you add a GNU changelog too? See: http://www.gnu.org/prep/standards/html_node/Change-Logs.html for the conventions. > Regards! > Hujp > > --- > gcc/opts.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/gcc/opts.c b/gcc/opts.c > index 499eb900643..dec5ba6d2be 100644 > --- a/gcc/opts.c > +++ b/gcc/opts.c > @@ -2007,10 +2007,20 @@ parse_and_check_align_values (const char *flag, > location LOC. */ > > static void > -check_alignment_argument (location_t loc, const char *flag, const char *name) > +check_alignment_argument (location_t loc, > + const char *flag, > + const char *name, > + int *opt_flag, > + const char **opt_str) The function comment needs to mention the new parameters. Maybe just add: OPT_STR points to the stored -falign-NAME= argument and OPT_FLAG points to the associated -falign-NAME on/off flag. */ It's more usual in GCC to put as many arguments on one line as possible, so maybe: check_alignment_argument (location_t loc, const char *flag, const char *name, int *opt_flag, const char **opt_str) > { > auto_vec<unsigned> align_result; > parse_and_check_align_values (flag, name, align_result, true, loc); > + > + if (align_result.length() >= 1 && align_result[0] == 0) > + { > + *opt_flag = 1; > + *opt_str = NULL; > + } The GCC style is to indent the braced “if” body by 2 more spaces: if (align_result.length() >= 1 && align_result[0] == 0) { *opt_flag = 1; *opt_str = NULL; } > } > > /* Print help when OPT__help_ is set. */ > @@ -2785,19 +2795,23 @@ common_handle_option (struct gcc_options *opts, > break; > > case OPT_falign_loops_: > - check_alignment_argument (loc, arg, "loops"); > + check_alignment_argument (loc, arg, "loops", > + &opts->x_flag_align_loops, &opts->x_str_align_loops); For multi-line argument lists, the second and subsequent lines should be indented under the first argument. So maybe something like: check_alignment_argument (loc, arg, "loops", &opts->x_flag_align_loops, &opts->x_str_align_loops); Thanks, Richard
On 7/27/20 9:46 AM, Hu Jiangping wrote: > Hi! > > This patch makes the -falign-foo=0 work as described in the > documentation. Thanks for all the suggestions, Richard and Segher! Hello. I'm the author of the original code. > > v3: make change more readable and self-consistent > v2: at a high level handles -falign-foo=0 like -falign-foo > > Regards! > Hujp > > --- > gcc/opts.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/gcc/opts.c b/gcc/opts.c > index 499eb900643..dec5ba6d2be 100644 > --- a/gcc/opts.c > +++ b/gcc/opts.c > @@ -2007,10 +2007,20 @@ parse_and_check_align_values (const char *flag, > location LOC. */ > > static void > -check_alignment_argument (location_t loc, const char *flag, const char *name) > +check_alignment_argument (location_t loc, > + const char *flag, > + const char *name, > + int *opt_flag, > + const char **opt_str) > { > auto_vec<unsigned> align_result; > parse_and_check_align_values (flag, name, align_result, true, loc); > + > + if (align_result.length() >= 1 && align_result[0] == 0) > + { > + *opt_flag = 1; > + *opt_str = NULL; > + } Hm, shouldn't the code be placed in parse_and_check_align_values? Note that there's one another call gcc/toplev.c (parse_N_M). I bet you likely want to modify result_values in case -falign-foo=0. About the -falign-foo=0:m:n2:m3, you can report an error in parse_and_check_align_values? Thanks for working on that? Btw. what's your use-case that you use the extended syntax of -falign-foo? Martin > } > > /* Print help when OPT__help_ is set. */ > @@ -2785,19 +2795,23 @@ common_handle_option (struct gcc_options *opts, > break; > > case OPT_falign_loops_: > - check_alignment_argument (loc, arg, "loops"); > + check_alignment_argument (loc, arg, "loops", > + &opts->x_flag_align_loops, &opts->x_str_align_loops); > break; > > case OPT_falign_jumps_: > - check_alignment_argument (loc, arg, "jumps"); > + check_alignment_argument (loc, arg, "jumps", > + &opts->x_flag_align_jumps, &opts->x_str_align_jumps); > break; > > case OPT_falign_labels_: > - check_alignment_argument (loc, arg, "labels"); > + check_alignment_argument (loc, arg, "labels", > + &opts->x_flag_align_labels, &opts->x_str_align_labels); > break; > > case OPT_falign_functions_: > - check_alignment_argument (loc, arg, "functions"); > + check_alignment_argument (loc, arg, "functions", > + &opts->x_flag_align_functions, &opts->x_str_align_functions); > break; > > case OPT_ftabstop_: >
Martin Liška <mliska@suse.cz> writes: > On 7/27/20 9:46 AM, Hu Jiangping wrote: >> Hi! >> >> This patch makes the -falign-foo=0 work as described in the >> documentation. Thanks for all the suggestions, Richard and Segher! > > Hello. > > I'm the author of the original code. > >> >> v3: make change more readable and self-consistent >> v2: at a high level handles -falign-foo=0 like -falign-foo >> >> Regards! >> Hujp >> >> --- >> gcc/opts.c | 24 +++++++++++++++++++----- >> 1 file changed, 19 insertions(+), 5 deletions(-) >> >> diff --git a/gcc/opts.c b/gcc/opts.c >> index 499eb900643..dec5ba6d2be 100644 >> --- a/gcc/opts.c >> +++ b/gcc/opts.c >> @@ -2007,10 +2007,20 @@ parse_and_check_align_values (const char *flag, >> location LOC. */ >> >> static void >> -check_alignment_argument (location_t loc, const char *flag, const char *name) >> +check_alignment_argument (location_t loc, >> + const char *flag, >> + const char *name, >> + int *opt_flag, >> + const char **opt_str) >> { >> auto_vec<unsigned> align_result; >> parse_and_check_align_values (flag, name, align_result, true, loc); >> + >> + if (align_result.length() >= 1 && align_result[0] == 0) >> + { >> + *opt_flag = 1; >> + *opt_str = NULL; >> + } > > Hm, shouldn't the code be placed in parse_and_check_align_values? Note that there's > one another call gcc/toplev.c (parse_N_M). I think that's why check_alignment_argument is the right place. The idea is that we're making -falign-foo=0 do two things: - imply -falign-foo - invalidate any previous -falign-foo=… option It's therefore something that we should only do while iterating through the command-line in order. > I bet you likely want to modify result_values in case -falign-foo=0. > About the -falign-foo=0:m:n2:m3, you can report an error in parse_and_check_align_values? The problem is that we don't know in general what the target's default is. It depends on whether this is aligning for functions, loops, labels, etc. It also depends on the target core, which we might not know yet, and which might change during compilation based on attributes and pragmas. So I don't think there's a different value that parse_and_check_align_values could sensibly insert instead of zero. Thanks, Richard
> > This patch makes the -falign-foo=0 work as described in the > > documentation. Thanks for all the suggestions, Richard and Segher! > > Hello. > > I'm the author of the original code. > > > > > v3: make change more readable and self-consistent > > v2: at a high level handles -falign-foo=0 like -falign-foo > > > > Regards! > > Hujp > > > > --- > > gcc/opts.c | 24 +++++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/gcc/opts.c b/gcc/opts.c > > index 499eb900643..dec5ba6d2be 100644 > > --- a/gcc/opts.c > > +++ b/gcc/opts.c > > @@ -2007,10 +2007,20 @@ parse_and_check_align_values (const char > *flag, > > location LOC. */ > > > > static void > > -check_alignment_argument (location_t loc, const char *flag, const char > *name) > > +check_alignment_argument (location_t loc, > > + const char *flag, > > + const char *name, > > + int *opt_flag, > > + const char **opt_str) > > { > > auto_vec<unsigned> align_result; > > parse_and_check_align_values (flag, name, align_result, true, loc); > > + > > + if (align_result.length() >= 1 && align_result[0] == 0) > > + { > > + *opt_flag = 1; > > + *opt_str = NULL; > > + } > > Hm, shouldn't the code be placed in parse_and_check_align_values? Note that > there's > one another call gcc/toplev.c (parse_N_M). > > I bet you likely want to modify result_values in case -falign-foo=0. > About the -falign-foo=0:m:n2:m3, you can report an error in > parse_and_check_align_values? > Thanks Martin! Reporting errors may break exist Makefiles and is not as good as handling -falign-foo=0 like -falign-foo. We have discussed earlier. And when parse_N_M is called, the values may have been overided by target code, I think it is inappropriate to report errors at that time. > Thanks for working on that? > Btw. what's your use-case that you use the extended syntax of -falign-foo? > Martin > In fact, I was understanding the cpu tuning structure of gcc, and then when I read the documentation of -falign-foo, I found this problem. Thanks, Hujp > > } > > > > /* Print help when OPT__help_ is set. */ > > @@ -2785,19 +2795,23 @@ common_handle_option (struct gcc_options > *opts, > > break; > > > > case OPT_falign_loops_: > > - check_alignment_argument (loc, arg, "loops"); > > + check_alignment_argument (loc, arg, "loops", > > + &opts->x_flag_align_loops, &opts->x_str_align_loops); > > break; > > > > case OPT_falign_jumps_: > > - check_alignment_argument (loc, arg, "jumps"); > > + check_alignment_argument (loc, arg, "jumps", > > + &opts->x_flag_align_jumps, &opts->x_str_align_jumps); > > break; > > > > case OPT_falign_labels_: > > - check_alignment_argument (loc, arg, "labels"); > > + check_alignment_argument (loc, arg, "labels", > > + &opts->x_flag_align_labels, &opts->x_str_align_labels); > > break; > > > > case OPT_falign_functions_: > > - check_alignment_argument (loc, arg, "functions"); > > + check_alignment_argument (loc, arg, "functions", > > + &opts->x_flag_align_functions, &opts->x_str_align_functions); > > break; > > > > case OPT_ftabstop_: > > > >
On 7/27/20 12:25 PM, Richard Sandiford wrote: > So I don't think there's a different value that parse_and_check_align_values > could sensibly insert instead of zero. All right, works for me. Martin
diff --git a/gcc/opts.c b/gcc/opts.c index 499eb900643..dec5ba6d2be 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -2007,10 +2007,20 @@ parse_and_check_align_values (const char *flag, location LOC. */ static void -check_alignment_argument (location_t loc, const char *flag, const char *name) +check_alignment_argument (location_t loc, + const char *flag, + const char *name, + int *opt_flag, + const char **opt_str) { auto_vec<unsigned> align_result; parse_and_check_align_values (flag, name, align_result, true, loc); + + if (align_result.length() >= 1 && align_result[0] == 0) + { + *opt_flag = 1; + *opt_str = NULL; + } } /* Print help when OPT__help_ is set. */ @@ -2785,19 +2795,23 @@ common_handle_option (struct gcc_options *opts, break; case OPT_falign_loops_: - check_alignment_argument (loc, arg, "loops"); + check_alignment_argument (loc, arg, "loops", + &opts->x_flag_align_loops, &opts->x_str_align_loops); break; case OPT_falign_jumps_: - check_alignment_argument (loc, arg, "jumps"); + check_alignment_argument (loc, arg, "jumps", + &opts->x_flag_align_jumps, &opts->x_str_align_jumps); break; case OPT_falign_labels_: - check_alignment_argument (loc, arg, "labels"); + check_alignment_argument (loc, arg, "labels", + &opts->x_flag_align_labels, &opts->x_str_align_labels); break; case OPT_falign_functions_: - check_alignment_argument (loc, arg, "functions"); + check_alignment_argument (loc, arg, "functions", + &opts->x_flag_align_functions, &opts->x_str_align_functions); break; case OPT_ftabstop_: