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 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;



-- 
H.J.