Re: [PATCH, i386] Fix -mpreferred-stack-boundary

[Sriraman Tallam <tmsriram at google dot com>]

On Tue, Nov 12, 2013 at 2:53 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
>
> On Tue, 12 Nov 2013 10:30:16, Sriraman Tallam wrote:
>>
>> On Mon, Nov 11, 2013 at 11:30 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>> There was something wrong with Bernd's address, retrying.
>>>
>>>>> Currently on trunk the option -mpreferred-stack-boundary does not work
>>>>> together with #pragma GCC target("sse") or __attribute__((target("sse"))).
>>>>>
>>>>> There is already a test case that detects this: gcc.target/i386/fastcall-sseregparm.c
>>>>>
>>>>> The attached patch fixes this test case under i686-pc-linux-gnu.
>>>>>
>>>>> Boot-strapped and regression-tested under i686-pc-linux-gnu.
>>>>>
>>>>> OK for trunk?

No, this is not what I had in mind. This is simply reverting my
refactoring work which was  to make ix86_option_override_internal get
rid of the global_options dependency.  Here is the problem:
global_options gets some flags set after command-line options are read
(ix86_preferred_stack_boundary_arg in this case).  But, this does not
get saved into target_option_default_node because there is no
corresponding field in cl_target_option for
ix86_preferred_stack_boundary_arg. So, when you restore
target_option_default_node to func_options in
ix86_valid_target_attribute_p, this particular flag does not get
copied. So, you can either copy this explicitly to func_options which
was your first patch or you could extend cl_target_option to include
this field too which is done by making
ix86_preferred_stack_boundary_arg a Variable in i386.opt. The latter
is cleaner because it always saves the default flags into
target_option_default_node.

Thanks
Sri


>>>>
>>>> I'm not experienced enough in this new option handling stuff, let's
>>>> ask Sriraman for his opinion on the patch.
>>
>>
>> I do not think this is the right fix, I am wondering how many other
>> target flags we may have to copy this way from global_options. I
>> notice that other flags like ix86_regparm and
>> ix86_incoming_stack_boundary_arg are very similar. Why should this
>> need to be restored from global_options explicitly? This patch may fix
>> the issue but it seems like a hack to me. We should be able to restore
>> whatever we need from target_option_default_node via
>> ix86_function_specific_restore. Maybe modifying the i386.opt file to
>> make ix86_preferred_stack_boundary_arg a variable might fix it. See
>> ix86_isa_flags for instance in i386.opt.
>>
>>
>> Please let me know what you think.
>
> Thanks, now I see what you mean. I can change it the other way round
> and implement ix86_preferred_stack_boundary like ix86_incoming_stack_boundary.
>
> using this define in options.h:
> #define ix86_preferred_stack_boundary_arg global_options.x_ix86_preferred_stack_boundary_arg
>
> the global option is never copied into function specific options.
>
> Attached is the updated patch.
>
> OK for trunk after boot-strap and regression-testing?
>
> Bernd.
>
>
> PS: I have one more patch pending, and would like to know what you think
> about it: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00133.html
> That has also to do with global state variables that are modified due to
> target options, and initially I was expecting that the patch for PR57756 would
> be fixing it automatically, but I was wrong...
>
>>
>> Thanks
>> Sri
>>
>>
>>>
>>> Uros.