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
>
> 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.
Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c (revision 204712)
+++ config/i386/i386.c (working copy)
@@ -4182,6 +4182,7 @@ ix86_function_specific_save (struct cl_target_opti
ptr->x_ix86_isa_flags_explicit = opts->x_ix86_isa_flags_explicit;
ptr->x_ix86_target_flags_explicit = opts->x_ix86_target_flags_explicit;
ptr->x_recip_mask_explicit = opts->x_recip_mask_explicit;
+ ptr->x_ix86_preferred_stack_boundary_arg =
opts->x_ix86_preferred_stack_boundary_arg;
/* The fields are char but the variables are not; make sure the
values fit in the fields. */
@@ -4211,6 +4212,7 @@ ix86_function_specific_restore (struct gcc_options
opts->x_ix86_isa_flags_explicit = ptr->x_ix86_isa_flags_explicit;
opts->x_ix86_target_flags_explicit = ptr->x_ix86_target_flags_explicit;
opts->x_recip_mask_explicit = ptr->x_recip_mask_explicit;
+ opts->x_ix86_preferred_stack_boundary_arg =
ptr->x_ix86_preferred_stack_boundary_arg;
/* Recreate the arch feature tests if the arch changed */
if (old_arch != ix86_arch)
Index: config/i386/i386.opt
===================================================================
--- config/i386/i386.opt (revision 204712)
+++ config/i386/i386.opt (working copy)
@@ -64,6 +64,12 @@ HOST_WIDE_INT x_ix86_isa_flags_explicit
Variable
int ix86_target_flags_explicit
+Variable
+int ix86_preferred_stack_boundary_arg
+
+TargetSave
+int x_ix86_preferred_stack_boundary_arg
+
;; which flags were passed by the user
TargetSave
HOST_WIDE_INT x_ix86_target_flags_explicit
