On Wed, Sep 14, 2011 at 10:20 AM, Tom de Vries <vr...@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 <t...@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.
