On Wed, Nov 13, 2013 at 5:12 PM, Bernd Edlinger
<bernd.edlin...@hotmail.de> wrote:
> 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.
That seems correct.
>
> 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.
This is correct. But, right now global_options_set is capturing only
the command line options that are set and does not seem to be
modified. If this option were to be made target specific we should not
access this field off global_options_set. We should add a MASK to
target flags and get it from there just like any other target flag
that is function specific does it.
> 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.
I understand your problem better now. I still do not think we should
make ix86_option_override_internal should read global_options flags
directly. That is overriding opts passed in as a parameter. I am fine
with Patch 1 which is explicitly copying global_options
preferred_stack_boundary_arg fields onto func_options. FYI, I cannot
approve any patches and you still have to get it approved by the
maintainers. I will sweep these copies and save it into
cl_target_option as a cleanup if more of these emerge.
Thanks for the patience,
Sri
>
> 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.
