Re: [PATCH 1/2] s/390: Implement "target" attribute.

[Dominik Vogt <vogt@linux.vnet.ibm.com>]

To improve readability, I'll split my answers (see below) into
several separate messages.

> > index 43459c8..4cf0df7 100644
> > --- a/gcc/common/config/s390/s390-common.c
> > +++ b/gcc/common/config/s390/s390-common.c
> > @@ -79,41 +79,27 @@ s390_option_init_struct (struct gcc_options *opts)
> >
> >  /* Implement TARGET_HANDLE_OPTION.  */
> >
> > -static bool
> > -s390_handle_option (struct gcc_options *opts,
> > +bool
> > +s390_handle_option (struct gcc_options *opts ATTRIBUTE_UNUSED,
> >  		    struct gcc_options *opts_set ATTRIBUTE_UNUSED,
> >    		    const struct cl_decoded_option *decoded,
> >  		    location_t loc)
> >  {
> >    size_t code = decoded->opt_index;
> > -  const char *arg = decoded->arg;
> >    int value = decoded->value;
> >
> >    switch (code)
> >      {
> > -    case OPT_march_:
> > -      opts->x_s390_arch_flags = processor_flags_table[value];
> > -      opts->x_s390_arch_string = arg;
> > -      return true;
> > -
> >      case OPT_mstack_guard_:
> > -      if (exact_log2 (value) == -1)
> > +      if (value != 0 && exact_log2 (value) == -1)
> >  	error_at (loc, "stack guard value must be an exact power of 2");
> >        return true;
> >
> >      case OPT_mstack_size_:
> > -      if (exact_log2 (value) == -1)
> > +      if (value != 0 && exact_log2 (value) == -1)
> >  	error_at (loc, "stack size must be an exact power of 2");
> >        return true;
> 
> This probably is supposed to allow disabling of stack_guard and
> stack-size options with 0 settings. Would removing the
> `RejectNegative' in s390.opt be an option?

The value 0 is meant to restore the default behaviour, i.e. switch
off -mstack-guard= or resotre the default stack size for
-mstack-size=.

If we remove the RejectNegative from the options, they still
require to be used as "-mno-stack-...=<number>".  The number is
ignored of course, and we'd have to add two more cases to the
function.  In other words, more code for less usability.

As an alternative, I can add something like this:

  mno-stack-guard 
  Target RejectNegative Alias(mstack-guard=,0) Negative(mstack-guard=) 

This installs -mno-stack-guard as an alias for -mstack-guard=0 and
provides the interface the user probably expects.  The patch above
is still necessary though.

--

But what the heck is this "exact power of 2" limitation good for
in the first place?  Why is a stack size of 1, 2 or
36028797018963968 valid, but not 800?  Shouldn't the stack size
(and the size of the stack guard) just be multiples of the stack
slot size?

> > +    memcpy (&options, &global_options, sizeof (options));
> options = global_options;

Oops.

> > +      /* Cast to int to avoid Warning "comparison is always true".  */
> What cast?

Sorry, just forgot to remove the comment when changing the code
last time.

> -      || new_opts_set.x_s390_warn_dynamicstack_p
> -      )
> +      || new_opts_set.x_s390_warn_dynamicstack_p)

Ok.

> -
>    return t;

Ok.

>  mtune=
> -Target RejectNegative Joined Enum(processor_type) Var(s390_tune) Init(PROCESSOR_max) Save.
> +Target RejectNegative Joined Enum(processor_type) Var(s390_tune) Init(PROCESSOR_max) Save
>  Schedule code for given CPU

The full stop should be at the end of the help text, of course.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany