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

[Ilya Enkovich]

2016-01-27 18:43 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
> On Wed, Jan 27, 2016 at 7:34 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> Hi,
>>
>> Currently STV pass may require a stack realignment if any
>> transformation occurs to enable SSE registers spill/fill.
>> It appears it's invalid to increase stack alignment requirements
>> at this point.  Thus we have to either assume we need stack to be
>> aligned if are going to run STV pass or disable STV if stack is
>> not properly aligned.  I suppose we shouldn't ignore explicitly
>> requested stack alignment not beeing sure we really optimize
>> anything (and STV is not an optimization frequiently applied).
>> So I think we may disable TARGET_STV for such cases as Jakub
>> suggested.  This patch was bootstrapped and regtested on
>> x86_64-pc-linux-gnu.  OK for trunk?
>>
>> Thanks,
>> 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,3} - 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;
>>    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;
>
> The right fix is
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index a03a515..62af55a 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;
>  }
>
> @@ -29300,8 +29290,10 @@ ix86_minimum_alignment (tree exp, machine_mode mode,
>      return align;
>
>    /* Don't do dynamic stack realignment for long long objects with
> -     -mpreferred-stack-boundary=2.  */
> -  if ((mode == DImode || (type && TYPE_MODE (type) == DImode))
> +     -mpreferred-stack-boundary=2.  The STV pass uses SSE2 instructions
> +     on DImode which needs 64-bit alignment for DImode.  */
> +  if (!(TARGET_STV && TARGET_SSE2 && optimize > 1)
> +      && (mode == DImode || (type && TYPE_MODE (type) == DImode))
>        && (!type || !TYPE_USER_ALIGN (type))
>        && (!decl || !DECL_USER_ALIGN (decl)))
>      return 32;
>

DImode object doesn't mean STV will be applied.  So you might just
ignore preferred stack
alignment for no reason. 'Right' here depends on what is more
important in such case.

Thanks,
Ilya

>
>
> --
> H.J.