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

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 415 bytes --]

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?

Regards
Bernd. 		 	   		  

[-- Attachment #2: changelog-pr58964.txt --]
[-- Type: text/plain, Size: 185 bytes --]

2013-11-10  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR target/58964
	* config/i386/i386.c (ix86_valid_target_attribute_p): Set
	func_options.x_ix86_preferred_stack_boundary_arg.


[-- Attachment #3: patch-pr58964.diff --]
[-- Type: application/octet-stream, Size: 668 bytes --]

Index: gcc/config/i386/i386.c
===================================================================
--- gcc/config/i386/i386.c	(revision 204101)
+++ gcc/config/i386/i386.c	(working copy)
@@ -4626,6 +4626,9 @@ ix86_valid_target_attribute_p (tree fndecl,
   memset (&func_options, 0, sizeof (func_options));
   init_options_struct (&func_options, NULL);
   lang_hooks.init_options_struct (&func_options);
+  if (global_options_set.x_ix86_preferred_stack_boundary_arg)
+    func_options.x_ix86_preferred_stack_boundary_arg
+      = global_options.x_ix86_preferred_stack_boundary_arg;
  
   cl_optimization_restore (&func_options,
 			   TREE_OPTIMIZATION (func_optimize));

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

[Sriraman Tallam]

On Fri, Nov 15, 2013 at 1:58 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On Fri, 15 Nov 2013 10:09:02, Uros Bizjak wrote:
>>
>> On Fri, Nov 15, 2013 at 4:54 AM, Sriraman Tallam <tmsriram@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.

I will prepare a patch to make these flags target specific, so that it
can get saved and restored in cl_target_option and does not depend on
global_options_set, and send a patch out next week.

Thanks
Sri


>
> Thanks
> Bernd.

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

[Uros Bizjak]

On Fri, Nov 15, 2013 at 10:58 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:

>>>> 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.

Do we have a pressing issue here that needs immediate, but incomplete fix?

If this is not the case, I'd rather wait for a correct fix.

Thanks,
Uros.

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

[Bernd Edlinger]

On Fri, 15 Nov 2013 10:09:02, Uros Bizjak wrote:
>
> On Fri, Nov 15, 2013 at 4:54 AM, Sriraman Tallam <tmsriram@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. 		 	   		  

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

[Uros Bizjak]

On Fri, Nov 15, 2013 at 4:54 AM, Sriraman Tallam <tmsriram@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.

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

[Sriraman Tallam]

On Wed, Nov 13, 2013 at 5:12 PM, Bernd Edlinger
<bernd.edlinger@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 <tmsriram@google.com> wrote:
>>> 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.
>>
>> 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.

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

[Bernd Edlinger]

On Tue, 12 Nov 2013 17:50:27, Sriraman Tallam wrote:
>
> On Tue, Nov 12, 2013 at 5:17 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>> 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.
>
> 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. 		 	   		  

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

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 3666 bytes --]

On Tue, Nov 12, 2013 at 5:17 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> 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.

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.

[-- Attachment #2: bernd_patch.txt --]
[-- Type: text/plain, Size: 1589 bytes --]

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

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

[Sriraman Tallam]

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.

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 2315 bytes --]

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?
>>>
>>> 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. 		 	   		  

[-- Attachment #2: changelog-pr58964.txt --]
[-- Type: text/plain, Size: 184 bytes --]

2013-11-12  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR target/58964
	* config/i386/i386.c (ix86_option_override_internal): Use
	global option ix86_preferred_stack_boundary_arg.


[-- Attachment #3: patch-pr58964.diff --]
[-- Type: application/octet-stream, Size: 1121 bytes --]

Index: gcc/config/i386/i386.c
===================================================================
--- gcc/config/i386/i386.c	(revision 204581)
+++ gcc/config/i386/i386.c	(working copy)
@@ -3735,19 +3735,19 @@ ix86_option_override_internal (bool main_args_p,
 		 ? (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)
+      if (ix86_preferred_stack_boundary_arg < min
+	  || ix86_preferred_stack_boundary_arg > max)
 	{
 	  if (min == max)
 	    error ("-mpreferred-stack-boundary is not supported "
 		   "for this target");
 	  else
 	    error ("-mpreferred-stack-boundary=%d is not between %d and %d",
-		   opts->x_ix86_preferred_stack_boundary_arg, min, max);
+		   ix86_preferred_stack_boundary_arg, min, max);
 	}
       else
 	ix86_preferred_stack_boundary
-	  = (1 << opts->x_ix86_preferred_stack_boundary_arg) * BITS_PER_UNIT;
+	  = (1 << ix86_preferred_stack_boundary_arg) * BITS_PER_UNIT;
     }
 
   /* Set the default value for -mstackrealign.  */

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

[Sriraman Tallam]

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?
>>
>> 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
Sri


>
> Uros.

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

[Uros Bizjak]

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?
>
> I'm not experienced enough in this new option handling stuff, let's
> ask Sriraman for his opinion on the patch.

Uros.

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

[Uros Bizjak]

Hello!

> 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?

I'm not experienced enough in this new option handling stuff, let's
ask Sriraman for his opinion on the patch.

Thanks,
Uros.