Re: [PATCH, PR target/69454] Disable TARGET_STV when stack is not properly aligned

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

2016-02-02 17:03 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
> On Tue, Feb 2, 2016 at 5:55 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2016-02-02 16:25 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>> On Tue, Feb 2, 2016 at 5:21 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>> 2016-02-02 16:14 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>> On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>> 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>>>> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>>>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>>>>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>>>>>>>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> >> The bottom line is  ix86_minimum_alignment must return the correct
>>>>>>>>>>>> >> number for DImode or you can just turn off STV.   My suggestion is
>>>>>>>>>>>> >> to use my patch.
>>>>>>>>>>>> >
>>>>>>>>>>>> > Uros, any preferences here?  I mean, it is possible to use
>>>>>>>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>>>>>>>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>>>>>>>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>>>>>>>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>>>>>>>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...)
>>>>>>>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>>>>>>>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>>>>>>>>>>>> > and that is something we don't want to affect.
>>>>>>>>>>>>
>>>>>>>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>>>>>>>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for
>>>>>>>>>>>> explicit -mstv in case stack boundary requirement is not satisfied.
>>>>>>>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can
>>>>>>>>>>>> revisit this decision for later gcc versions.
>>>>>>>>>>>>
>>>>>>>>>>>> I think disabling STV is less surprising option than increasing stack
>>>>>>>>>>>> boundary behind the user's back.
>>>>>>>>>>>
>>>>>>>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>>>>>>>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)?
>>>>>>>>>>> I believe it does just that.
>>>>>>>>>>
>>>>>>>>>> This patch is WRONG.
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> H.J.
>>>>>>>>>
>>>>>>>>> You will run into the same ICE with
>>>>>>>>>
>>>>>>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>>>>>>>
>>>>>>>>> in a leaf function which needs DImode spill/fill.
>>>>>>>>
>>>>>>>> Why would we need DImode spill/fill having no DImode registers?
>>>>>>>>
>>>>>>>
>>>>>>> Because STV is enabled with
>>>>>>>
>>>>>>>  -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>>>>
>>>>>> I misread it as -mpreferred-... So why would we fail having a proper
>>>>>> preferred stack alignment? AFAIK leaf function doesn't affect
>>>>>> alignment until we finalize it after RA.
>>>>>>
>>>>>
>>>>> /* Finalize stack_realign_needed flag, which will guide prologue/epilogue
>>>>>    to be generated in correct form.  */
>>>>> static void
>>>>> ix86_finalize_stack_realign_flags (void)
>>>>> {
>>>>>   /* Check if stack realign is really needed after reload, and
>>>>>      stores result in cfun */
>>>>>   unsigned int incoming_stack_boundary
>>>>>     = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary
>>>>>        ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary);
>>>>>   unsigned int stack_realign
>>>>>     = (incoming_stack_boundary
>>>>>        < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor
>>>>>           ? crtl->max_used_stack_slot_alignment
>>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>
>>>> We call it after RA when all spill slots are allocated and check if we
>>>> may relax stack alignment. Don't see any problem here.
>>>
>>> Please see
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69454#c26
>>>
>>> Why did LRA crash then?
>>
>> Because it tries a patch [1] which doesn't fix stack alignment and STV
>> enabling and therefore doesn't resolve the problem when
>> -mpreferred-stack-boundary=2 is used.
>>
>
> No, it is because RA doesn't increase stack alignment.  You have to
> have the correct stack alignment requirement before entering RA.

And it's too late to do it after STV pass and therefore we disable it
when stack is not properly aligned. I think this argumentation goes in
a loop.

Thanks,
Ilya

>
>
> --
> H.J.