Fix --help -Q output

Message ID 3f7bc23c-6daf-3d99-7ae7-4575a9d2659a@suse.cz
State New
Headers
Series Fix --help -Q output |

Commit Message

Martin Liška Nov. 29, 2021, 3:16 p.m. UTC
  There are cases where a default option value is -1 and
auto-detection happens in e.g. target.

Do not print these options. Leads to the following diff:

-  -fdelete-null-pointer-checks 		[enabled]
+  -fdelete-null-pointer-checks 		
@@ -332 +332 @@
-  -fleading-underscore        		[enabled]
+  -fleading-underscore        		
@@ -393 +393 @@
-  -fprefetch-loop-arrays      		[enabled]
+  -fprefetch-loop-arrays      		
@@ -502 +502 @@
-  -fstrict-volatile-bitfields 		[enabled]
+  -fstrict-volatile-bitfields 		
@@ -533 +533 @@
-  -ftree-loop-if-convert      		[enabled]
+  -ftree-loop-if-convert      		

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

	PR middle-end/103438

gcc/ChangeLog:

	* opts-common.c (option_enabled): Return flag_var for BOOLEAN
	types (the can contain an unknown value -1).
---
  gcc/opts-common.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)
  

Comments

Richard Biener Nov. 30, 2021, 9:33 a.m. UTC | #1
On Mon, Nov 29, 2021 at 4:16 PM Martin Liška <mliska@suse.cz> wrote:
>
> There are cases where a default option value is -1 and
> auto-detection happens in e.g. target.
>
> Do not print these options. Leads to the following diff:
>
> -  -fdelete-null-pointer-checks                 [enabled]
> +  -fdelete-null-pointer-checks
> @@ -332 +332 @@
> -  -fleading-underscore                 [enabled]
> +  -fleading-underscore
> @@ -393 +393 @@
> -  -fprefetch-loop-arrays               [enabled]
> +  -fprefetch-loop-arrays
> @@ -502 +502 @@
> -  -fstrict-volatile-bitfields          [enabled]
> +  -fstrict-volatile-bitfields
> @@ -533 +533 @@
> -  -ftree-loop-if-convert               [enabled]
> +  -ftree-loop-if-convert
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
>
>         PR middle-end/103438
>
> gcc/ChangeLog:
>
>         * opts-common.c (option_enabled): Return flag_var for BOOLEAN
>         types (the can contain an unknown value -1).
> ---
>   gcc/opts-common.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/opts-common.c b/gcc/opts-common.c
> index 9d1914ff2ff..c4a19b9a0b6 100644
> --- a/gcc/opts-common.c
> +++ b/gcc/opts-common.c
> @@ -1586,7 +1586,8 @@ option_flag_var (int opt_index, struct gcc_options *opts)
>   }
>
>   /* Return 1 if option OPT_IDX is enabled in OPTS, 0 if it is disabled,
> -   or -1 if it isn't a simple on-off switch.  */
> +   or -1 if it isn't a simple on-off switch (or if the value is unknown,
> +   typically set later in target).  */
>
>   int
>   option_enabled (int opt_idx, unsigned lang_mask, void *opts)
> @@ -1608,9 +1609,9 @@ option_enabled (int opt_idx, unsigned lang_mask, void *opts)
>         {
>         case CLVC_BOOLEAN:
>         if (option->cl_host_wide_int)
> -         return *(HOST_WIDE_INT *) flag_var != 0;
> +         return *(HOST_WIDE_INT *) flag_var;
>         else
> -         return *(int *) flag_var != 0;
> +         return *(int *) flag_var;

So can we assert that only 1, 0 and -1 are returned here?  For example I see
(random picked)

 /* [64] = */ {
    "--param=align-threshold=",
    "Select fraction of the maximal frequency of executions of basic
block in function given basic block get alignment.",
    NULL,
    NULL,
    NULL, NULL, N_OPTS, N_OPTS, 23, /* .neg_idx = */ -1,
    CL_COMMON | CL_JOINED | CL_OPTIMIZATION | CL_PARAMS,
    0, 0, 0, 0, 0, 0, 0, 0, 1 /* UInteger */, 0, 0, 0,
    offsetof (struct gcc_options, x_param_align_threshold), 0,
CLVC_BOOLEAN, 0, 1, 65536 },

or

 /* [877] = */ {
    "-faligned-new=",
    "-faligned-new=<N>  Use C++17 over-aligned type allocation for
alignments greater than N.",
    NULL,
    NULL,
    NULL, NULL, N_OPTS, N_OPTS, 13, /* .neg_idx = */ -1,
    CL_CXX | CL_ObjCXX | CL_JOINED,
    0, 0, 0, 0, 0, 0, 1 /* RejectNegative */, 0, 1 /* UInteger */, 0, 0, 0,
    offsetof (struct gcc_options, x_aligned_new_threshold), 0,
CLVC_BOOLEAN, 0, -1, -1 },

and the "docs" say

  /* The switch is enabled when FLAG_VAR is nonzero.  */
  CLVC_BOOLEAN,

so a -1 init contradicts this.  I also wonder what determines the
CLVC_* kind, it seems
it's simply the "default" kind chosen when nothing else matches in var_set().

>
>         case CLVC_EQUAL:
>         if (option->cl_host_wide_int)
> --
> 2.34.0
>
  
Martin Liška Dec. 2, 2021, 2:07 p.m. UTC | #2
On 11/30/21 10:33, Richard Biener wrote:
> and the "docs" say
> 
>    /* The switch is enabled when FLAG_VAR is nonzero.  */
>    CLVC_BOOLEAN,

That's bogus, because real meaning of CLVC_BOOLEAN is that it holds an integer value.
It can be just true/false for a simple flag, can be 0,1,2 for things like flag_lifetime_dse,
or it can be an arbitrary integer value for things like params.

That said, I would suggest removing that..

Thoughts about the patch?
Martin
  
Richard Biener Dec. 2, 2021, 2:11 p.m. UTC | #3
On Thu, Dec 2, 2021 at 3:07 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 11/30/21 10:33, Richard Biener wrote:
> > and the "docs" say
> >
> >    /* The switch is enabled when FLAG_VAR is nonzero.  */
> >    CLVC_BOOLEAN,
>
> That's bogus, because real meaning of CLVC_BOOLEAN is that it holds an integer value.
> It can be just true/false for a simple flag, can be 0,1,2 for things like flag_lifetime_dse,
> or it can be an arbitrary integer value for things like params.
>
> That said, I would suggest removing that..
>
> Thoughts about the patch?

+           return v > 0 ? (v < 0 ? -1 : 1) : 0;

the ?:s look odd to me, the above is equivalent to v >  0 ? 1 : 0, no?
Did you mean to make v > 0 v != 0?

> Martin
  
Martin Liška Dec. 2, 2021, 2:22 p.m. UTC | #4
On 12/2/21 15:11, Richard Biener wrote:
> the ?:s look odd to me, the above is equivalent to v >  0 ? 1 : 0, no?
> Did you mean to make v > 0 v != 0?

Yeah, it's typo, should be 'v != 0 ? ...'.

Martin
  
Martin Liška Dec. 2, 2021, 3:55 p.m. UTC | #5
On 12/2/21 15:22, Martin Liška wrote:
> On 12/2/21 15:11, Richard Biener wrote:
>> the ?:s look odd to me, the above is equivalent to v >  0 ? 1 : 0, no?
>> Did you mean to make v > 0 v != 0?
> 
> Yeah, it's typo, should be 'v != 0 ? ...'.
> 
> Martin

There's a tested patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin
  
Richard Biener Dec. 7, 2021, 12:24 p.m. UTC | #6
On Thu, Dec 2, 2021 at 4:55 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 12/2/21 15:22, Martin Liška wrote:
> > On 12/2/21 15:11, Richard Biener wrote:
> >> the ?:s look odd to me, the above is equivalent to v >  0 ? 1 : 0, no?
> >> Did you mean to make v > 0 v != 0?
> >
> > Yeah, it's typo, should be 'v != 0 ? ...'.
> >
> > Martin
>
> There's a tested patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

OK.

Thanks,
Richard.

> Thanks,
> Martin
  

Patch

diff --git a/gcc/opts-common.c b/gcc/opts-common.c
index 9d1914ff2ff..c4a19b9a0b6 100644
--- a/gcc/opts-common.c
+++ b/gcc/opts-common.c
@@ -1586,7 +1586,8 @@  option_flag_var (int opt_index, struct gcc_options *opts)
  }
  
  /* Return 1 if option OPT_IDX is enabled in OPTS, 0 if it is disabled,
-   or -1 if it isn't a simple on-off switch.  */
+   or -1 if it isn't a simple on-off switch (or if the value is unknown,
+   typically set later in target).  */
  
  int
  option_enabled (int opt_idx, unsigned lang_mask, void *opts)
@@ -1608,9 +1609,9 @@  option_enabled (int opt_idx, unsigned lang_mask, void *opts)
        {
        case CLVC_BOOLEAN:
  	if (option->cl_host_wide_int)
-	  return *(HOST_WIDE_INT *) flag_var != 0;
+	  return *(HOST_WIDE_INT *) flag_var;
  	else
-	  return *(int *) flag_var != 0;
+	  return *(int *) flag_var;
  
        case CLVC_EQUAL:
  	if (option->cl_host_wide_int)