On Fri, 15 Nov 2013 10:09:02, Uros Bizjak wrote:
>
> On Fri, Nov 15, 2013 at 4:54 AM, Sriraman Tallam <tmsri...@google.com> wrote:
>
>>>>>>>>>> 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.
>
> I'd prefer the proposed general solution. It seems to me that Patch 1
> is somehow a hack that will "solve" only one particular option out of
> many similar.
>
> Thanks,
> Uros.
I agree, if you want, I can apply patch #2 today, (which at least does not
look so hacky as my first approach), to buy us some time.
And I think Sriram should prepare a followup-patch that removes
this dependencies on global_options_set or makes that data structure
also target specific.
Thanks
Bernd.
