On Tue, 12 Nov 2013 17:50:27, Sriraman Tallam wrote:
>
> On Tue, Nov 12, 2013 at 5:17 PM, Sriraman Tallam <tmsri...@google.com> wrote:
>> On Tue, Nov 12, 2013 at 2:53 PM, Bernd Edlinger
>> <bernd.edlin...@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 <ubiz...@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.
>
> I quickly hacked up what I had in mind and attached the patch. Can you
> check if this fixes your problem?
>
> Thanks
> Sri
>
>
Well, this way it could be fixed too.
But opts->x_ix86_preferred_stack_bounary_arg is not dependent on any
pragma or target attribute. Correct me if that is wrong.
And this code
if (opts_set->x_ix86_preferred_stack_boundary_arg)
{
int min = (TARGET_64BIT_P (opts->x_ix86_isa_flags)
? (TARGET_SSE_P (opts->x_ix86_isa_flags) ? 4 : 3) : 2);
int max = (TARGET_SEH ? 4 : 12);
if (opts->x_ix86_preferred_stack_boundary_arg < min
|| opts->x_ix86_preferred_stack_boundary_arg> max)
checks func_options against global_options_set:
new_target = ix86_valid_target_attribute_tree (args, &func_options,
&global_options_set);
So this code as it is will fail if this option was ever made target specific.
There is still a reason why this check needs to be executed each time
the opts->x_ix86_isa_flags changes.
Because of this I still would prefer my second attempt of fixing this issue,
because it is simple and it removes the different handling between
-mpreferred-stack-boundary and -mincoming-stack-boundary.
If that should be re-factored for any reason, I think all similar options
should be changed on one sweep. But in that case the global_options_set
must somehow also become target specific.
And we need to invent something like a target pragma to change this options
because it must somehow be possible to test this code.
Thanks
Bernd.
>>
>> 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.
