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

[Dominik Vogt <vogt at linux dot vnet dot ibm dot com>]

> > +/* Define platform dependent macros.  */
> > +void
> > +s390_cpu_cpp_builtins (cpp_reader *pfile)
> > +{
> > +  struct cl_target_option opts;
> > +
> > +  cpp_assert (pfile, "cpu=s390");
> > +  cpp_assert (pfile, "machine=s390");
> > +  cpp_define (pfile, "__s390__");
> > +  if (TARGET_ZARCH)
> > +    cpp_define (pfile, "__zarch__");
> > +  if (TARGET_64BIT)
> > +    cpp_define (pfile, "__s390x__");
> > +  if (TARGET_LONG_DOUBLE_128)
> > +    cpp_define (pfile, "__LONG_DOUBLE_128__");
> > +  cl_target_option_save (&opts, &global_options);
> > +  s390_cpu_cpp_builtins_internal (pfile, &opts, NULL);
> > +}
> > +
> > +#if S390_USE_TARGET_ATTRIBUTE
> > +/* Hook to validate the current #pragma GCC target and set the state, and
> > +   update the macros based on what was changed.  If ARGS is NULL, then
> > +   POP_TARGET is used to reset the options.  */
> > +
> > +static bool
> > +s390_pragma_target_parse (tree args, tree pop_target)
> > +{
> > +  tree prev_tree = build_target_option_node (&global_options);
> > +  tree cur_tree;
> > +
> > +  {
> > +    gcc_options options;
> > +    tree def_tree;
> > +
> > +    memcpy (&options, &global_options, sizeof (options));
> options = global_options;
> 
> > +    def_tree = (pop_target ? pop_target : target_option_default_node);
> > +    cl_target_option_restore (&options, TREE_TARGET_OPTION (def_tree));
> > +    if (! args)
> > +      cur_tree = def_tree;
> > +    else
> > +      {
> > +	gcc_options options_set;
> > +
> > +	memcpy (&options_set, &global_options_set, sizeof (options_set));
> options_set = global_options_set;
> 
> > +	cur_tree = s390_valid_target_attribute_tree (args, &options,
> > +						     &options_set, true);
> > +	if (!cur_tree || cur_tree == error_mark_node)
> > +	  return false;
> Don't you need to do:
> global_options_set = options_set;
> 
> > +      }
> > +    memcpy (&global_options, &options, sizeof (options));
> global_options = options;
> 
> All the backup and restore of the option structures is probably done
> here to prevent half-way parsed options from staying in the data
> structures while running into an error - right?!  Is this really a
> problem, e.g. the i386 back-end does not seem to do it?

I386 also has a backup in prev_tree, but we have that too, so
"options" can be removed.  Note that if parsing fails on i386, it
still overwrites global_opts_set and never restores it.

What this code does is somewhat strange anyway.  According to the
documentation, the pragma works as if any following functions had
its options wrapped in a target attribute.  So, the pragma should
only have an effect on functions, but *actually* the code
immediately modifies the global_opts structure when the pragma is
parsed.  I wonder if this is intentional.

> > +  if (old_tree != new_tree)
> > +    {
> > +      cl_target_option_restore (&global_options, TREE_TARGET_OPTION (new_tree));
> > +      if (TREE_TARGET_GLOBALS (new_tree))
> > +	restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
> > +      else if (new_tree == target_option_default_node)
> > +	restore_target_globals (&default_target_globals);
> > +      else
> > +	TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts ();
> 
> This last part is almost s390_reset_previous_fndecl. Perhaps it could
> be merged?!

Done.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany