Patchwork [PR50251] set DRAP type stack realignment for stack_restore

login
register
mail settings
Submitter Tom de Vries
Date Sept. 14, 2011, 8:20 a.m.
Message ID <4E7063CF.7080505@codesourcery.com>
Download mbox | patch
Permalink /patch/114589/
State New
Headers show

Comments

Tom de Vries - Sept. 14, 2011, 8:20 a.m.
On 09/04/2011 03:44 PM, Tom de Vries wrote:
> On 09/04/2011 11:10 AM, Richard Guenther wrote:
>> On Sun, Sep 4, 2011 at 11:00 AM, Tom de Vries <vries@codesourcery.com> wrote:
>>> Hi,
>>>
>>> this patch fixes PR50251, which was caused by r178353.
>>>
>>> The patch was bootstrapped and reg-tested on i686 and x86_64.
>>> On i686, the test-cases reported failing in PR50251 pass again.
>>>
>>> The patch selects the DRAP type stack realignment method in case a stack_restore
>>> is used. If that is not done, the presence of the stack_restore at reload leaves
>>> FRAME_POINTER without an elimination rule for i386 port.
>>>
>>> OK for trunk?
>>
>> Shouldn't we eventually simply set cfun->calls_alloca when expanding either
>> stack save or restore?  Similar to how it is set from
>> allocate_dynamic_stack_space?
>> I'm not sure we encountered the dead stack save/restore pair before
>> this new folding,
>> so, just to make other targets not confused about them as well?
> 
> Setting cfun->calls_alloca will work as well, but doesn't seem necessary to me.
> AFAIU, since other targets don't define MAX_STACK_ALIGNMENT, they don't need to
> do a realign themselves. If I disable MAX_STACK_ALIGNMENT on i386, the
> middle-end handles the realign and the stack_restore causes no problems. So to
> me this seems a problem with the i386-specific implementation of realignment.
> 
> I'm more worried about other similar cases not working for i386 than about other
> ports. Another, more involved way to fix this, would be in reload to:
> - calculate which registers we cannot use for elimination (which we already do)
> - pass that as context to target.can_eliminate. The i386 target can then
>   fulfill its requirement to be able to eliminate the frame pointer by
>   setting need_drap if the stack pointer is not available.
> I think this way we could remove most if not all of the
> 'crtl->need_drap = true' snippets, and have a completer solution and perhaps
> also more optimal.
> 
> Is this simple crtl->need_drap fix ok for now, or should I start testing the
> cfun->calls_alloca fix?
> 

Ping.

I have tested the attached patch on both x86_64 and i686 and it fixes all
failures in PR50251 without any regressions.

Since I don't feel like the right person to dig into reload guts to implement
the solution above, I want to check in this fix to i386 backend.

Uros, Richard H., OK for trunk?

Thank you,
- Tom

2011-09-14  Tom de Vries  <tom@codesourcery.com>

	* explow.c (emit_stack_restore): Set crtl->need_drap if
	stack_restore is emitted.
Uros Bizjak - Sept. 14, 2011, 9:57 a.m.
On Wed, Sep 14, 2011 at 10:20 AM, Tom de Vries <vries@codesourcery.com> wrote:

>>>> this patch fixes PR50251, which was caused by r178353.
>>>>
>>>> The patch was bootstrapped and reg-tested on i686 and x86_64.
>>>> On i686, the test-cases reported failing in PR50251 pass again.
>>>>
>>>> The patch selects the DRAP type stack realignment method in case a stack_restore
>>>> is used. If that is not done, the presence of the stack_restore at reload leaves
>>>> FRAME_POINTER without an elimination rule for i386 port.
>>>>
>>>> OK for trunk?
>>>
>>> Shouldn't we eventually simply set cfun->calls_alloca when expanding either
>>> stack save or restore?  Similar to how it is set from
>>> allocate_dynamic_stack_space?
>>> I'm not sure we encountered the dead stack save/restore pair before
>>> this new folding,
>>> so, just to make other targets not confused about them as well?
>>
>> Setting cfun->calls_alloca will work as well, but doesn't seem necessary to me.
>> AFAIU, since other targets don't define MAX_STACK_ALIGNMENT, they don't need to
>> do a realign themselves. If I disable MAX_STACK_ALIGNMENT on i386, the
>> middle-end handles the realign and the stack_restore causes no problems. So to
>> me this seems a problem with the i386-specific implementation of realignment.
>>
>> I'm more worried about other similar cases not working for i386 than about other
>> ports. Another, more involved way to fix this, would be in reload to:
>> - calculate which registers we cannot use for elimination (which we already do)
>> - pass that as context to target.can_eliminate. The i386 target can then
>>   fulfill its requirement to be able to eliminate the frame pointer by
>>   setting need_drap if the stack pointer is not available.
>> I think this way we could remove most if not all of the
>> 'crtl->need_drap = true' snippets, and have a completer solution and perhaps
>> also more optimal.
>>
>> Is this simple crtl->need_drap fix ok for now, or should I start testing the
>> cfun->calls_alloca fix?
>>
>
> Ping.
>
> I have tested the attached patch on both x86_64 and i686 and it fixes all
> failures in PR50251 without any regressions.
>
> Since I don't feel like the right person to dig into reload guts to implement
> the solution above, I want to check in this fix to i386 backend.
>
> Uros, Richard H., OK for trunk?
>
> Thank you,
> - Tom
>
> 2011-09-14  Tom de Vries  <tom@codesourcery.com>
>
>        * explow.c (emit_stack_restore): Set crtl->need_drap if
>        stack_restore is emitted.

Since this approach just follows the approach taken in other builtin_*
functions, I'd say this is an oversight for stack_save/restore
builtins and the patch is OK. Any enhancements in this area can be
implemented independently of the fix.

Please also add the test from PR, with -mpreferred-stack-boundary=12
that currently fails for 32bit and 64bit x86 targets.

Thanks,
Uros.

Patch

Index: gcc/explow.c
===================================================================
--- gcc/explow.c (revision 178145)
+++ gcc/explow.c (working copy)
@@ -1062,6 +1062,20 @@  emit_stack_restore (enum save_level save
   /* The default is that we use a move insn.  */
   rtx (*fcn) (rtx, rtx) = gen_move_insn;
 
+  /* If stack_realign_drap, the x86 backend emits a prologue that aligns both
+     STACK_POINTER and HARD_FRAME_POINTER.
+     If stack_realign_fp, the x86 backend emits a prologue that aligns only
+     STACK_POINTER. This renders the HARD_FRAME_POINTER unusable for accessing
+     aligned variables, which is reflected in ix86_can_eliminate.
+     We normally still have the realigned STACK_POINTER that we can use.
+     But if there is a stack restore still present at reload, it can trigger 
+     mark_not_eliminable for the STACK_POINTER, leaving no way to eliminate
+     FRAME_POINTER into a hard reg.
+     To prevent this situation, we force need_drap if we emit a stack
+     restore.  */
+  if (SUPPORTS_STACK_ALIGNMENT)
+    crtl->need_drap = true;
+
   /* See if this machine has anything special to do for this kind of save.  */
   switch (save_level)
     {