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 Thu, Jan 28, 2016 at 2:06 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2016-01-28 9:00 GMT+03:00 H.J. Lu <hjl.tools@gmail.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.
>
> TARGET_STV doesn't mean STV pass will run. We can check alignment in STV
> pass gate and this assert would be wrong. If we decide STV to be dependent on
> stack alignment then we shouldn't make alignment be dependent on STV. I can add
> assert into convert_scalars_to_vector to check
> crtl->stack_alignment_estimated >= 64
> by that moment.
>

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.


-- 
H.J.