Re: [PATCH, PR50251] set DRAP type stack realignment for stack_restore

[Uros Bizjak <ubizjak at gmail dot com>]

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.