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

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

On Wed, Jan 27, 2016 at 8:36 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Jan 27, 2016 at 8:29 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2016-01-27 19:18 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>> On Wed, Jan 27, 2016 at 8:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>> On 27 Jan 16:44, Jakub Jelinek wrote:
>>>>> On Wed, Jan 27, 2016 at 06:34:41PM +0300, Ilya Enkovich wrote:
>>>>> > @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p,
>>>>> >      opts->x_target_flags |= MASK_VZEROUPPER;
>>>>> >    if (!(opts_set->x_target_flags & MASK_STV))
>>>>> >      opts->x_target_flags |= MASK_STV;
>>>>> > +  /* Disable STV if -mpreferred-stack-boundary={2,3} - the needed
>>>>>
>>>>> The comment doesn't match the code, you disable STV only for
>>>>> -mpreferred-stack-boundary=2.
>>>>
>>>> Thanks, here is an updated version.
>>>>
>>>> Ilya
>>>> --
>>>> gcc/
>>>>
>>>> 2016-01-27  Jakub Jelinek  <jakub@redhat.com>
>>>>             Ilya Enkovich  <enkovich.gnu@gmail.com>
>>>>
>>>>         PR target/69454
>>>>         * config/i386/i386.c (convert_scalars_to_vector): Remove
>>>>         stack alignment fixes.
>>>>         (ix86_option_override_internal): Disable TARGET_STV if stack
>>>>         is not properly aligned.
>>>>
>>>> gcc/testsuite/
>>>>
>>>> 2016-01-27  Ilya Enkovich  <enkovich.gnu@gmail.com>
>>>>
>>>>         PR target/69454
>>>>         * gcc.target/i386/pr69454-1.c: New test.
>>>>         * gcc.target/i386/pr69454-2.c: New test.
>>>>
>>>>
>>>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>>>> index 34b57a4..9fb8db8 100644
>>>> --- a/gcc/config/i386/i386.c
>>>> +++ b/gcc/config/i386/i386.c
>>>> @@ -3588,16 +3588,6 @@ convert_scalars_to_vector ()
>>>>    bitmap_obstack_release (NULL);
>>>>    df_process_deferred_rescans ();
>>>>
>>>> -  /* Conversion means we may have 128bit register spills/fills
>>>> -     which require aligned stack.  */
>>>> -  if (converted_insns)
>>>> -    {
>>>> -      if (crtl->stack_alignment_needed < 128)
>>>> -       crtl->stack_alignment_needed = 128;
>>>> -      if (crtl->stack_alignment_estimated < 128)
>>>> -       crtl->stack_alignment_estimated = 128;
>>>> -    }
>>>> -
>>>>    return 0;
>>>>  }
>>>>
>>>> @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p,
>>>>      opts->x_target_flags |= MASK_VZEROUPPER;
>>>>    if (!(opts_set->x_target_flags & MASK_STV))
>>>>      opts->x_target_flags |= MASK_STV;
>>>> +  /* Disable STV if -mpreferred-stack-boundary=2 - the needed
>>>> +     stack realignment will be extra cost the pass doesn't take into
>>>> +     account and the pass can't realign the stack.  */
>>>> +  if (ix86_preferred_stack_boundary < 64)
>>>> +    opts->x_target_flags &= ~MASK_STV;

This won't work for 32-bit incoming stack boundary and 64-bit preferred
stack boundary.  In this case, STV won't be off.  When LRA needs 64-bit
aligned stack slot, stack must be realigned.  But for leaf function, we may
not realign stack if ix86_minimum_alignment returns 32 for DImode.   You
must either add assert (!TARGET_STV) before returning 32 for DImode or
return 64 for DImode if STV is on in ix86_minimum_alignment.

>>>>    if (!ix86_tune_features[X86_TUNE_AVX256_UNALIGNED_LOAD_OPTIMAL]
>>>>        && !(opts_set->x_target_flags & MASK_AVX256_SPLIT_UNALIGNED_LOAD))
>>>>      opts->x_target_flags |= MASK_AVX256_SPLIT_UNALIGNED_LOAD;
>>>
>>> MINIMUM_ALIGNMENT keeps track stack alignment.  It is OK
>>> to disable STV for -mpreferred-stack-boundary=2.  But you should
>>> also update ix86_minimum_alignment to make sure that STV is
>>> disabled before returning 32 for DImode.
>>
>> If -mpreferred-stack-boundary=2 then STV is disabled, if STV is enabled then
>> -mpreferred-stack-boundary>=3 and this condition in
>> ix86_minimum_alignment works:
>>
>>   if (TARGET_64BIT || align != 64 || ix86_preferred_stack_boundary >= 64)
>>     return align;
>>
>
> No, you shouldn't make assumptions in ix86_minimum_alignment. You
> should check explicitly that STV is disabled in ix86_minimum_alignment.
>
>
> --
> H.J.



-- 
H.J.