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

[Uros Bizjak <ubizjak@gmail.com>]

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

Uros.