[v3] driver: fix a problem with implementation of -falign-foo=0 [PR96247]

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]
Related show

Commit Message

Hu, Jiangping July 27, 2020, 7:46 a.m.
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(-)

-- 
2.17.1

Comments

Hu, Jiangping July 27, 2020, 7:57 a.m. | #1
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

> 

>
Richard Sandiford July 27, 2020, 8:53 a.m. | #2
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
Martin Liška July 27, 2020, 9:40 a.m. | #3
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_:

>
Richard Sandiford July 27, 2020, 10:25 a.m. | #4
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
Hu, Jiangping July 27, 2020, 10:36 a.m. | #5
> > 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_:

> >

> 

>
Martin Liška July 27, 2020, 10:53 a.m. | #6
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

Patch

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_: