Re: PATCH: PR target/40838: gcc shouldn't assume that the stack is aligned

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Thu, Oct 15, 2009 at 12:51 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On 10/15/2009 09:32 PM, H.J. Lu wrote:
>>>>>
>>>>> We should use PREFERRED_STACK_BOUNDARY_DEFAULT instead of hardcoded 128
>>>>> here. At least the comment for P_S_B_D says:
>>>>>
>>>>> /* It should be MIN_STACK_BOUNDARY. ïBut we set it to 128 bits for
>>>>> ï both 32bit and 64bit, to support codes that need 128 bit stack
>>>>> ï alignment for SSE instructions, but can't realign the stack. ï*/
>>>>> #define PREFERRED_STACK_BOUNDARY_DEFAULT 128
>>>>>
>>>>>
>>>>
>>>> In forced_stack_alignment, we collect the hard alignment requirement
>>>> on stack. The decision if stack should be aligned depends on the
>>>> incoming stack alignment and the hard stack alignment. It is separate
>>>> from PREFERRED_STACK_BOUNDARY_DEFAULT.
>>>>
>>>>
>>>>
>>>>>
>>>>> OTOH, I think that this whole stuff should depend on -mstackrealing
>>>>> somehow,
>>>>> according to the comment:
>>>>>
>>>>> /* 1 if -mstackrealign should be turned on by default. ïIt will
>>>>> ï generate an alternate prologue and epilogue that realigns the
>>>>> ï runtime stack if nessary. ïThis supports mixing codes that keep a
>>>>> ï 4-byte aligned stack, as specified by i386 psABI, with codes that
>>>>> ï need a 16-byte aligned stack, as required by SSE instructions. ïIf
>>>>> ï STACK_REALIGN_DEFAULT is 1 and PREFERRED_STACK_BOUNDARY_DEFAULT is
>>>>> ï 128, stacks for all functions may be realigned. ï*/
>>>>> #define STACK_REALIGN_DEFAULT 0
>>>>>
>>>>> As far as I understand from many comments from the PR40838 trail
>>>>> (especially
>>>>> comment #51), -mstackrealing is not effective in some cases involving
>>>>> automatic SSE variables on the stack. I think that -mstackrealign
>>>>> should
>>>>> be
>>>>> fixed in the way your patch outlines, so old/broken sources that assume
>>>>> 4byte alignment can be compiled using this option without penalizing
>>>>> new/fixed code that assumes 16byte alignment.
>>>>>
>>>>>
>>>>>
>>>>
>>>> -mstackrealign is very effective. If we turn it on by default, it works
>>>> fine:
>>>>
>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40838#c45
>>>>
>>>> It is just that we don't want to make it the default since we will
>>>> realign
>>>> the stack for almost all functions. In most cases, misaligned stack
>>>> won't
>>>> cause any problems.
>>>>
>>>> What my patch does is to assume 4byte incoming stack alignment
>>>> for functions containing SSE instructions which require hard 16byte
>>>> stack alignment.
>>>>
>>>> After my patch is applied, we can update -mstackrealign to make it an
>>>> no-op since all the problems it tries to solve:
>>>>
>>>> http://gcc.gnu.org/ml/gcc-patches/2006-02/msg00854.html
>>>> http://gcc.gnu.org/ml/gcc-patches/2006-05/msg00526.html
>>>>
>>>> disappeared with my patch.
>>>>
>>>>
>>>
>>> Then we should do realignment the other way around: instead of using
>>> -mstackrealing for all the code (including where it has no effect), let's
>>> use -mstackrealign to activate realignment functionality that is
>>> introduced
>>> by your patch.
>>>
>>
>> That defeats the whole purpose of my patch, which automatically
>> realigns the stack when there is a hard alignment requirement. If it
>> isn't turned on by default, it is not very useful.
>>
>>
>>>
>>> IOW, lightweight -mstackrealign, firing up only when there is the
>>> possibility of unaligned access in the code it precedes.
>>>
>>>
>>
>> That is what my patch does, but turned it on by default.
>>
>
> I think that we have the same situation here as was with the infamous -mcld
> option. A specific functionality is needed for compatibility with some
> [broken] code and this functionality has non-negligible impact on the
> performance. AFAICS, there will always be opinions for this functionality as

Well, performance impact of my patch on SPEC CPU 2006 is pretty much
in the noise.

> well as opinions against it.
>
> I propose that we implement new option -mhard-stackrealign [we can bikeshed
> about this option name a bit ;) ] with corresponding
> --enable-hard-stackrealign as a configure option. This way, both groups can
> have whatever they prefer - compatibility vs. performance.
>
> Looking at -mcld discussions, is this acceptable solution?
>

Is --enable-hard-stackrealign enabled by default. If not, it doesn't
buy much.


-- 
H.J.