2016-01-28 9:00 GMT+03:00 H.J. Lu <hjl.to...@gmail.com>:
> On Wed, Jan 27, 2016 at 8:36 AM, H.J. Lu <hjl.to...@gmail.com> wrote:
>> On Wed, Jan 27, 2016 at 8:29 AM, Ilya Enkovich <enkovich....@gmail.com>
>> wrote:
>>> 2016-01-27 19:18 GMT+03:00 H.J. Lu <hjl.to...@gmail.com>:
>>>> On Wed, Jan 27, 2016 at 8:11 AM, Ilya Enkovich <enkovich....@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 <ja...@redhat.com>
>>>>> Ilya Enkovich <enkovich....@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....@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.
Thanks,
Ilya
>
>>>>> 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.
