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

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

[Ilya Enkovich]

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;
diff --git a/gcc/testsuite/gcc.target/i386/pr69454-1.c b/gcc/testsuite/gcc.target/i386/pr69454-1.c
new file mode 100644
index 0000000..12ecfd3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr69454-1.c
@@ -0,0 +1,11 @@
+/* { dg-do compile { target { ia32 } } } */
+/* { dg-options "-O2 -msse2 -mno-accumulate-outgoing-args -mpreferred-stack-boundary=2" } */
+
+typedef struct { long long w64[2]; } V128;
+extern V128* fn2(void);
+long long a;
+V128 b;
+void fn1() {
+  V128 *c = fn2();
+  c->w64[0] = a ^ b.w64[0];
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr69454-2.c b/gcc/testsuite/gcc.target/i386/pr69454-2.c
new file mode 100644
index 0000000..28bab93
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr69454-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile { target { ia32 } } } */
+/* { dg-options "-O2 -mpreferred-stack-boundary=2" } */
+
+extern void fn2 ();
+long long a, b;
+
+void fn1 ()
+{
+  long long c = a;
+  a = b ^ a;
+  fn2 ();
+  a = c;
+}

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

[H.J. Lu]

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.

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

[Jakub Jelinek]

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.

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

	Jakub

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1988 bytes --]

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

> diff --git a/gcc/testsuite/gcc.target/i386/pr69454-1.c b/gcc/testsuite/gcc.target/i386/pr69454-1.c
> new file mode 100644
> index 0000000..12ecfd3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr69454-1.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile { target { ia32 } } } */
> +/* { dg-options "-O2 -msse2 -mno-accumulate-outgoing-args -mpreferred-stack-boundary=2" } */
> +
> +typedef struct { long long w64[2]; } V128;
> +extern V128* fn2(void);
> +long long a;
> +V128 b;
> +void fn1() {
> +  V128 *c = fn2();
> +  c->w64[0] = a ^ b.w64[0];
> +}
> diff --git a/gcc/testsuite/gcc.target/i386/pr69454-2.c b/gcc/testsuite/gcc.target/i386/pr69454-2.c
> new file mode 100644
> index 0000000..28bab93
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr69454-2.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile { target { ia32 } } } */
> +/* { dg-options "-O2 -mpreferred-stack-boundary=2" } */

This needs:

+/* { dg-options "-O2 -msse2 -mno-accumulate-outgoing-args
-mpreferred-stack-boundary=2" } */

to trigger.

> +extern void fn2 ();
> +long long a, b;
> +
> +void fn1 ()
> +{
> +  long long c = a;
> +  a = b ^ a;
> +  fn2 ();
> +  a = c;
> +}

Here is a different patch, which I believe is the right fix.

-- 
H.J.

[-- Attachment #2: 0001-Don-t-align-DImode-to-32-bits-if-the-STV-pass-is-ena.patch --]
[-- Type: text/x-patch, Size: 3312 bytes --]

From 86e040399dd5ca6b23597be4aff5edb9ac2ab5d7 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 25 Jan 2016 12:31:45 -0800
Subject: [PATCH] Don't align DImode to 32 bits if the STV pass is enabled

Since the STV pass uses SSE2 instructions on DImode which needs 64-bit
alignment for DImode, don't align DImode to 32 bits if the STV pass is
enabled.

gcc/

	PR target/69454
	* config/i386/i386.c (convert_scalars_to_vector): Don't change
	stack alignment here.
	(ix86_minimum_alignment): Don't align DImode to 32 bits if the
	STV pass is enabled.

gcc/testsuite/

	PR target/69454
	* gcc.target/i386/pr69454-1.c: New test.
	* gcc.target/i386/pr69454-2.c: Likewise.
---
 gcc/config/i386/i386.c                    | 16 ++++------------
 gcc/testsuite/gcc.target/i386/pr69454-1.c | 11 +++++++++++
 gcc/testsuite/gcc.target/i386/pr69454-2.c | 13 +++++++++++++
 3 files changed, 28 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr69454-1.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr69454-2.c

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index cfbdf0f..8babdaf 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;
 }
 
@@ -29299,8 +29289,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;
diff --git a/gcc/testsuite/gcc.target/i386/pr69454-1.c b/gcc/testsuite/gcc.target/i386/pr69454-1.c
new file mode 100644
index 0000000..12ecfd3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr69454-1.c
@@ -0,0 +1,11 @@
+/* { dg-do compile { target { ia32 } } } */
+/* { dg-options "-O2 -msse2 -mno-accumulate-outgoing-args -mpreferred-stack-boundary=2" } */
+
+typedef struct { long long w64[2]; } V128;
+extern V128* fn2(void);
+long long a;
+V128 b;
+void fn1() {
+  V128 *c = fn2();
+  c->w64[0] = a ^ b.w64[0];
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr69454-2.c b/gcc/testsuite/gcc.target/i386/pr69454-2.c
new file mode 100644
index 0000000..4820b76
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr69454-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile { target { ia32 } } } */
+/* { dg-options "-O2 -msse2 -mno-accumulate-outgoing-args -mpreferred-stack-boundary=2" } */
+
+extern void fn2 (void);
+long long a, b;
+void
+fn1 (void)
+{
+  long long c = a;
+  a = b ^ a;
+  fn2 ();
+  a = c;
+}
-- 
2.5.0

Re: [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.

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

[Ilya Enkovich]

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;
   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;
diff --git a/gcc/testsuite/gcc.target/i386/pr69454-1.c b/gcc/testsuite/gcc.target/i386/pr69454-1.c
new file mode 100644
index 0000000..12ecfd3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr69454-1.c
@@ -0,0 +1,11 @@
+/* { dg-do compile { target { ia32 } } } */
+/* { dg-options "-O2 -msse2 -mno-accumulate-outgoing-args -mpreferred-stack-boundary=2" } */
+
+typedef struct { long long w64[2]; } V128;
+extern V128* fn2(void);
+long long a;
+V128 b;
+void fn1() {
+  V128 *c = fn2();
+  c->w64[0] = a ^ b.w64[0];
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr69454-2.c b/gcc/testsuite/gcc.target/i386/pr69454-2.c
new file mode 100644
index 0000000..28bab93
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr69454-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile { target { ia32 } } } */
+/* { dg-options "-O2 -mpreferred-stack-boundary=2" } */
+
+extern void fn2 ();
+long long a, b;
+
+void fn1 ()
+{
+  long long c = a;
+  a = b ^ a;
+  fn2 ();
+  a = c;
+}

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

[H.J. Lu]

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


-- 
H.J.

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

[Ilya Enkovich]

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

Thanks,
Ilya

>
>
> --
> H.J.

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

[H.J. Lu]

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

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

[H.J. Lu]

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.

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

[Ilya Enkovich]

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.

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.

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

[H.J. Lu]

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.

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

[Jakub Jelinek]

On Thu, Jan 28, 2016 at 04:42:02AM -0800, H.J. Lu wrote:
> >>>>>> 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.

Uros, any preferences here?  I mean, it is possible to use
e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
change as a safety net, in the usual case for -mpreferred-stack-boundary=2
we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
anything, as TARGET_STV will be false, and if for whatever case it gets
through (target attribute, -mincoming-stack-boundary=, ...)
ix86_minimum_alignment will be there to ensure enough stack alignment.
Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
and that is something we don't want to affect.

	Jakub

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

[Uros Bizjak]

On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:

>> 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.
>
> Uros, any preferences here?  I mean, it is possible to use
> e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
> change as a safety net, in the usual case for -mpreferred-stack-boundary=2
> we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
> anything, as TARGET_STV will be false, and if for whatever case it gets
> through (target attribute, -mincoming-stack-boundary=, ...)
> ix86_minimum_alignment will be there to ensure enough stack alignment.
> Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
> and that is something we don't want to affect.

IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
is only an optimization. Perhaps we can also emit a "sorry" for
explicit -mstv in case stack boundary requirement is not satisfied.
*If* there is a need for -mstv with smaller stack boundary, we can
revisit this decision for later gcc versions.

I think disabling STV is less surprising option than increasing stack
boundary behind the user's back.

Uros.

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

[Jakub Jelinek]

On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> >> 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.
> >
> > Uros, any preferences here?  I mean, it is possible to use
> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
> > anything, as TARGET_STV will be false, and if for whatever case it gets
> > through (target attribute, -mincoming-stack-boundary=, ...)
> > ix86_minimum_alignment will be there to ensure enough stack alignment.
> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
> > and that is something we don't want to affect.
> 
> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
> is only an optimization. Perhaps we can also emit a "sorry" for
> explicit -mstv in case stack boundary requirement is not satisfied.
> *If* there is a need for -mstv with smaller stack boundary, we can
> revisit this decision for later gcc versions.
> 
> I think disabling STV is less surprising option than increasing stack
> boundary behind the user's back.

So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
ok for trunk then (alone or with additional sorry, incremental or not?)?
I believe it does just that.

	Jakub

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

[H.J. Lu]

On Tue, Feb 2, 2016 at 4:24 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>
>>> 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.
>>
>> Uros, any preferences here?  I mean, it is possible to use
>> e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>> change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>> we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>> anything, as TARGET_STV will be false, and if for whatever case it gets
>> through (target attribute, -mincoming-stack-boundary=, ...)
>> ix86_minimum_alignment will be there to ensure enough stack alignment.
>> Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>> and that is something we don't want to affect.
>
> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
> is only an optimization. Perhaps we can also emit a "sorry" for
> explicit -mstv in case stack boundary requirement is not satisfied.
> *If* there is a need for -mstv with smaller stack boundary, we can
> revisit this decision for later gcc versions.
>
> I think disabling STV is less surprising option than increasing stack
> boundary behind the user's back.
>
> Uros.

My ix86_minimum_alignment change is for correctness, which is required for
DImode spill/fill unless LRA supports unaligned spill/fill.   Disabling
TARGET_STV for  -mpreferred-stack-boundary=2 is an optimization.

Let me point it out again.  Checking  -mpreferred-stack-boundary < 3 to
disable STV won't work for 32-bit incoming stack boundary and 64-bit preferred
stack boundary.  In this case, STV is on.  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.   We must return 64 for DImode
if STV is on in ix86_minimum_alignment.

-- 
H.J.

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

[H.J. Lu]

On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> >> 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.
>> >
>> > Uros, any preferences here?  I mean, it is possible to use
>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>> > through (target attribute, -mincoming-stack-boundary=, ...)
>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>> > and that is something we don't want to affect.
>>
>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>> is only an optimization. Perhaps we can also emit a "sorry" for
>> explicit -mstv in case stack boundary requirement is not satisfied.
>> *If* there is a need for -mstv with smaller stack boundary, we can
>> revisit this decision for later gcc versions.
>>
>> I think disabling STV is less surprising option than increasing stack
>> boundary behind the user's back.
>
> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
> ok for trunk then (alone or with additional sorry, incremental or not?)?
> I believe it does just that.

This patch is WRONG.

-- 
H.J.

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

[H.J. Lu]

On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>
>>> >> 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.
>>> >
>>> > Uros, any preferences here?  I mean, it is possible to use
>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>>> > through (target attribute, -mincoming-stack-boundary=, ...)
>>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>>> > and that is something we don't want to affect.
>>>
>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>>> is only an optimization. Perhaps we can also emit a "sorry" for
>>> explicit -mstv in case stack boundary requirement is not satisfied.
>>> *If* there is a need for -mstv with smaller stack boundary, we can
>>> revisit this decision for later gcc versions.
>>>
>>> I think disabling STV is less surprising option than increasing stack
>>> boundary behind the user's back.
>>
>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>> ok for trunk then (alone or with additional sorry, incremental or not?)?
>> I believe it does just that.
>
> This patch is WRONG.
>
> --
> H.J.

You will run into the same ICE with

-mincoming-stack-boundary=2 -msse2 -O2 -m32

in a leaf function which needs DImode spill/fill.


-- 
H.J.

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

[Ilya Enkovich]

2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>
>>>> >> 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.
>>>> >
>>>> > Uros, any preferences here?  I mean, it is possible to use
>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>>>> > through (target attribute, -mincoming-stack-boundary=, ...)
>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>>>> > and that is something we don't want to affect.
>>>>
>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>>>> is only an optimization. Perhaps we can also emit a "sorry" for
>>>> explicit -mstv in case stack boundary requirement is not satisfied.
>>>> *If* there is a need for -mstv with smaller stack boundary, we can
>>>> revisit this decision for later gcc versions.
>>>>
>>>> I think disabling STV is less surprising option than increasing stack
>>>> boundary behind the user's back.
>>>
>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>>> ok for trunk then (alone or with additional sorry, incremental or not?)?
>>> I believe it does just that.
>>
>> This patch is WRONG.
>>
>> --
>> H.J.
>
> You will run into the same ICE with
>
> -mincoming-stack-boundary=2 -msse2 -O2 -m32
>
> in a leaf function which needs DImode spill/fill.

Why would we need DImode spill/fill having no DImode registers?

Thanks,
Ilya

>
>
> --
> H.J.

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

[H.J. Lu]

On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>
>>>>> >> 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.
>>>>> >
>>>>> > Uros, any preferences here?  I mean, it is possible to use
>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>>>>> > through (target attribute, -mincoming-stack-boundary=, ...)
>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>>>>> > and that is something we don't want to affect.
>>>>>
>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>>>>> is only an optimization. Perhaps we can also emit a "sorry" for
>>>>> explicit -mstv in case stack boundary requirement is not satisfied.
>>>>> *If* there is a need for -mstv with smaller stack boundary, we can
>>>>> revisit this decision for later gcc versions.
>>>>>
>>>>> I think disabling STV is less surprising option than increasing stack
>>>>> boundary behind the user's back.
>>>>
>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>>>> ok for trunk then (alone or with additional sorry, incremental or not?)?
>>>> I believe it does just that.
>>>
>>> This patch is WRONG.
>>>
>>> --
>>> H.J.
>>
>> You will run into the same ICE with
>>
>> -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>
>> in a leaf function which needs DImode spill/fill.
>
> Why would we need DImode spill/fill having no DImode registers?
>

Because STV is enabled with

 -mincoming-stack-boundary=2 -msse2 -O2 -m32


-- 
H.J.

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

[Uros Bizjak]

On Tue, Feb 2, 2016 at 2:06 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>
>>>>>> >> 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.
>>>>>> >
>>>>>> > Uros, any preferences here?  I mean, it is possible to use
>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...)
>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>>>>>> > and that is something we don't want to affect.
>>>>>>
>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for
>>>>>> explicit -mstv in case stack boundary requirement is not satisfied.
>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can
>>>>>> revisit this decision for later gcc versions.
>>>>>>
>>>>>> I think disabling STV is less surprising option than increasing stack
>>>>>> boundary behind the user's back.
>>>>>
>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)?
>>>>> I believe it does just that.
>>>>
>>>> This patch is WRONG.
>>>>
>>>> --
>>>> H.J.
>>>
>>> You will run into the same ICE with
>>>
>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>
>>> in a leaf function which needs DImode spill/fill.
>>
>> Why would we need DImode spill/fill having no DImode registers?
>>
>
> Because STV is enabled with
>
>  -mincoming-stack-boundary=2 -msse2 -O2 -m32

But this is the whole trick. Since stack alignment requirements won't
be satisfied, we disable STV even with -msse2.

Uros.

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

[Jakub Jelinek]

On Tue, Feb 02, 2016 at 04:46:26AM -0800, H.J. Lu wrote:
> >> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
> >> ok for trunk then (alone or with additional sorry, incremental or not?)?
> >> I believe it does just that.
> >
> > This patch is WRONG.
> >
> > --
> > H.J.
> 
> You will run into the same ICE with
> 
> -mincoming-stack-boundary=2 -msse2 -O2 -m32
> 
> in a leaf function which needs DImode spill/fill.

So are you arguing for changing
+  /* 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;
to
+  /* Disable STV if -m{preferred,incoming}-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
+      || ix86_incoming_stack_boundary < 64)
+    opts->x_target_flags &= ~MASK_STV;
I'm fine with that.

	Jakub

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

[Ilya Enkovich]

2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>
>>>>>> >> 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.
>>>>>> >
>>>>>> > Uros, any preferences here?  I mean, it is possible to use
>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...)
>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>>>>>> > and that is something we don't want to affect.
>>>>>>
>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for
>>>>>> explicit -mstv in case stack boundary requirement is not satisfied.
>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can
>>>>>> revisit this decision for later gcc versions.
>>>>>>
>>>>>> I think disabling STV is less surprising option than increasing stack
>>>>>> boundary behind the user's back.
>>>>>
>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)?
>>>>> I believe it does just that.
>>>>
>>>> This patch is WRONG.
>>>>
>>>> --
>>>> H.J.
>>>
>>> You will run into the same ICE with
>>>
>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>
>>> in a leaf function which needs DImode spill/fill.
>>
>> Why would we need DImode spill/fill having no DImode registers?
>>
>
> Because STV is enabled with
>
>  -mincoming-stack-boundary=2 -msse2 -O2 -m32

I misread it as -mpreferred-... So why would we fail having a proper
preferred stack alignment? AFAIK leaf function doesn't affect
alignment until we finalize it after RA.

>
>
> --
> H.J.

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

[H.J. Lu]

On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>>
>>>>>>> >> 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.
>>>>>>> >
>>>>>>> > Uros, any preferences here?  I mean, it is possible to use
>>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...)
>>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>>>>>>> > and that is something we don't want to affect.
>>>>>>>
>>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for
>>>>>>> explicit -mstv in case stack boundary requirement is not satisfied.
>>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can
>>>>>>> revisit this decision for later gcc versions.
>>>>>>>
>>>>>>> I think disabling STV is less surprising option than increasing stack
>>>>>>> boundary behind the user's back.
>>>>>>
>>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)?
>>>>>> I believe it does just that.
>>>>>
>>>>> This patch is WRONG.
>>>>>
>>>>> --
>>>>> H.J.
>>>>
>>>> You will run into the same ICE with
>>>>
>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>>
>>>> in a leaf function which needs DImode spill/fill.
>>>
>>> Why would we need DImode spill/fill having no DImode registers?
>>>
>>
>> Because STV is enabled with
>>
>>  -mincoming-stack-boundary=2 -msse2 -O2 -m32
>
> I misread it as -mpreferred-... So why would we fail having a proper
> preferred stack alignment? AFAIK leaf function doesn't affect
> alignment until we finalize it after RA.
>

/* Finalize stack_realign_needed flag, which will guide prologue/epilogue
   to be generated in correct form.  */
static void
ix86_finalize_stack_realign_flags (void)
{
  /* Check if stack realign is really needed after reload, and
     stores result in cfun */
  unsigned int incoming_stack_boundary
    = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary
       ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary);
  unsigned int stack_realign
    = (incoming_stack_boundary
       < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor
          ? crtl->max_used_stack_slot_alignment
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

For leaf function, we check max_used_stack_slot_alignment.
Since ix86_minimum_alignment returns 32 for DImode.
We won't realign stack for DImode spill/fill.

          : crtl->stack_alignment_needed));


-- 
H.J.

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

[H.J. Lu]

On Tue, Feb 2, 2016 at 5:09 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Feb 02, 2016 at 04:46:26AM -0800, H.J. Lu wrote:
>> >> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>> >> ok for trunk then (alone or with additional sorry, incremental or not?)?
>> >> I believe it does just that.
>> >
>> > This patch is WRONG.
>> >
>> > --
>> > H.J.
>>
>> You will run into the same ICE with
>>
>> -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>
>> in a leaf function which needs DImode spill/fill.
>
> So are you arguing for changing
> +  /* 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;
> to
> +  /* Disable STV if -m{preferred,incoming}-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
> +      || ix86_incoming_stack_boundary < 64)
> +    opts->x_target_flags &= ~MASK_STV;
> I'm fine with that.
>

There are many checks/asserts for stack alignment.  At minimum,
we should assert STV is off in ix86_minimum_alignment before
returning 32 for DImode.


-- 
H.J.

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

[Ilya Enkovich]

2016-02-02 16:14 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
> On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>>>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>>>
>>>>>>>> >> 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.
>>>>>>>> >
>>>>>>>> > Uros, any preferences here?  I mean, it is possible to use
>>>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>>>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>>>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>>>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>>>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...)
>>>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>>>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>>>>>>>> > and that is something we don't want to affect.
>>>>>>>>
>>>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>>>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for
>>>>>>>> explicit -mstv in case stack boundary requirement is not satisfied.
>>>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can
>>>>>>>> revisit this decision for later gcc versions.
>>>>>>>>
>>>>>>>> I think disabling STV is less surprising option than increasing stack
>>>>>>>> boundary behind the user's back.
>>>>>>>
>>>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>>>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)?
>>>>>>> I believe it does just that.
>>>>>>
>>>>>> This patch is WRONG.
>>>>>>
>>>>>> --
>>>>>> H.J.
>>>>>
>>>>> You will run into the same ICE with
>>>>>
>>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>>>
>>>>> in a leaf function which needs DImode spill/fill.
>>>>
>>>> Why would we need DImode spill/fill having no DImode registers?
>>>>
>>>
>>> Because STV is enabled with
>>>
>>>  -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>
>> I misread it as -mpreferred-... So why would we fail having a proper
>> preferred stack alignment? AFAIK leaf function doesn't affect
>> alignment until we finalize it after RA.
>>
>
> /* Finalize stack_realign_needed flag, which will guide prologue/epilogue
>    to be generated in correct form.  */
> static void
> ix86_finalize_stack_realign_flags (void)
> {
>   /* Check if stack realign is really needed after reload, and
>      stores result in cfun */
>   unsigned int incoming_stack_boundary
>     = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary
>        ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary);
>   unsigned int stack_realign
>     = (incoming_stack_boundary
>        < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor
>           ? crtl->max_used_stack_slot_alignment
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

We call it after RA when all spill slots are allocated and check if we
may relax stack alignment. Don't see any problem here.

Thanks,
Ilya

>
> For leaf function, we check max_used_stack_slot_alignment.
> Since ix86_minimum_alignment returns 32 for DImode.
> We won't realign stack for DImode spill/fill.
>
>           : crtl->stack_alignment_needed));
>
>
> --
> H.J.

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

[H.J. Lu]

On Tue, Feb 2, 2016 at 5:21 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2016-02-02 16:14 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>> On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>> 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>>>>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>>>>
>>>>>>>>> >> 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.
>>>>>>>>> >
>>>>>>>>> > Uros, any preferences here?  I mean, it is possible to use
>>>>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>>>>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>>>>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>>>>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>>>>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...)
>>>>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>>>>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>>>>>>>>> > and that is something we don't want to affect.
>>>>>>>>>
>>>>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>>>>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for
>>>>>>>>> explicit -mstv in case stack boundary requirement is not satisfied.
>>>>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can
>>>>>>>>> revisit this decision for later gcc versions.
>>>>>>>>>
>>>>>>>>> I think disabling STV is less surprising option than increasing stack
>>>>>>>>> boundary behind the user's back.
>>>>>>>>
>>>>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>>>>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)?
>>>>>>>> I believe it does just that.
>>>>>>>
>>>>>>> This patch is WRONG.
>>>>>>>
>>>>>>> --
>>>>>>> H.J.
>>>>>>
>>>>>> You will run into the same ICE with
>>>>>>
>>>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>>>>
>>>>>> in a leaf function which needs DImode spill/fill.
>>>>>
>>>>> Why would we need DImode spill/fill having no DImode registers?
>>>>>
>>>>
>>>> Because STV is enabled with
>>>>
>>>>  -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>
>>> I misread it as -mpreferred-... So why would we fail having a proper
>>> preferred stack alignment? AFAIK leaf function doesn't affect
>>> alignment until we finalize it after RA.
>>>
>>
>> /* Finalize stack_realign_needed flag, which will guide prologue/epilogue
>>    to be generated in correct form.  */
>> static void
>> ix86_finalize_stack_realign_flags (void)
>> {
>>   /* Check if stack realign is really needed after reload, and
>>      stores result in cfun */
>>   unsigned int incoming_stack_boundary
>>     = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary
>>        ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary);
>>   unsigned int stack_realign
>>     = (incoming_stack_boundary
>>        < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor
>>           ? crtl->max_used_stack_slot_alignment
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> We call it after RA when all spill slots are allocated and check if we
> may relax stack alignment. Don't see any problem here.

Please see

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69454#c26

Why did LRA crash then?


-- 
H.J.

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

[Ilya Enkovich]

2016-02-02 16:25 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
> On Tue, Feb 2, 2016 at 5:21 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2016-02-02 16:14 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>> On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>> 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>>>>>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>>>>>
>>>>>>>>>> >> 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.
>>>>>>>>>> >
>>>>>>>>>> > Uros, any preferences here?  I mean, it is possible to use
>>>>>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>>>>>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>>>>>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>>>>>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>>>>>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...)
>>>>>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>>>>>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>>>>>>>>>> > and that is something we don't want to affect.
>>>>>>>>>>
>>>>>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>>>>>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for
>>>>>>>>>> explicit -mstv in case stack boundary requirement is not satisfied.
>>>>>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can
>>>>>>>>>> revisit this decision for later gcc versions.
>>>>>>>>>>
>>>>>>>>>> I think disabling STV is less surprising option than increasing stack
>>>>>>>>>> boundary behind the user's back.
>>>>>>>>>
>>>>>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>>>>>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)?
>>>>>>>>> I believe it does just that.
>>>>>>>>
>>>>>>>> This patch is WRONG.
>>>>>>>>
>>>>>>>> --
>>>>>>>> H.J.
>>>>>>>
>>>>>>> You will run into the same ICE with
>>>>>>>
>>>>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>>>>>
>>>>>>> in a leaf function which needs DImode spill/fill.
>>>>>>
>>>>>> Why would we need DImode spill/fill having no DImode registers?
>>>>>>
>>>>>
>>>>> Because STV is enabled with
>>>>>
>>>>>  -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>>
>>>> I misread it as -mpreferred-... So why would we fail having a proper
>>>> preferred stack alignment? AFAIK leaf function doesn't affect
>>>> alignment until we finalize it after RA.
>>>>
>>>
>>> /* Finalize stack_realign_needed flag, which will guide prologue/epilogue
>>>    to be generated in correct form.  */
>>> static void
>>> ix86_finalize_stack_realign_flags (void)
>>> {
>>>   /* Check if stack realign is really needed after reload, and
>>>      stores result in cfun */
>>>   unsigned int incoming_stack_boundary
>>>     = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary
>>>        ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary);
>>>   unsigned int stack_realign
>>>     = (incoming_stack_boundary
>>>        < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor
>>>           ? crtl->max_used_stack_slot_alignment
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> We call it after RA when all spill slots are allocated and check if we
>> may relax stack alignment. Don't see any problem here.
>
> Please see
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69454#c26
>
> Why did LRA crash then?

Because it tries a patch [1] which doesn't fix stack alignment and STV
enabling and therefore doesn't resolve the problem when
-mpreferred-stack-boundary=2 is used.

Thanks,
Ilya
--
[1] https://gcc.gnu.org/bugzilla/attachment.cgi?id=37468&action=diff

>
>
> --
> H.J.

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

[H.J. Lu]

On Tue, Feb 2, 2016 at 5:55 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2016-02-02 16:25 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>> On Tue, Feb 2, 2016 at 5:21 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>> 2016-02-02 16:14 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>> On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>> 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>>> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>>>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>>>>>>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> >> 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.
>>>>>>>>>>> >
>>>>>>>>>>> > Uros, any preferences here?  I mean, it is possible to use
>>>>>>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>>>>>>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>>>>>>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>>>>>>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>>>>>>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...)
>>>>>>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>>>>>>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>>>>>>>>>>> > and that is something we don't want to affect.
>>>>>>>>>>>
>>>>>>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>>>>>>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for
>>>>>>>>>>> explicit -mstv in case stack boundary requirement is not satisfied.
>>>>>>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can
>>>>>>>>>>> revisit this decision for later gcc versions.
>>>>>>>>>>>
>>>>>>>>>>> I think disabling STV is less surprising option than increasing stack
>>>>>>>>>>> boundary behind the user's back.
>>>>>>>>>>
>>>>>>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>>>>>>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)?
>>>>>>>>>> I believe it does just that.
>>>>>>>>>
>>>>>>>>> This patch is WRONG.
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> H.J.
>>>>>>>>
>>>>>>>> You will run into the same ICE with
>>>>>>>>
>>>>>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>>>>>>
>>>>>>>> in a leaf function which needs DImode spill/fill.
>>>>>>>
>>>>>>> Why would we need DImode spill/fill having no DImode registers?
>>>>>>>
>>>>>>
>>>>>> Because STV is enabled with
>>>>>>
>>>>>>  -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>>>
>>>>> I misread it as -mpreferred-... So why would we fail having a proper
>>>>> preferred stack alignment? AFAIK leaf function doesn't affect
>>>>> alignment until we finalize it after RA.
>>>>>
>>>>
>>>> /* Finalize stack_realign_needed flag, which will guide prologue/epilogue
>>>>    to be generated in correct form.  */
>>>> static void
>>>> ix86_finalize_stack_realign_flags (void)
>>>> {
>>>>   /* Check if stack realign is really needed after reload, and
>>>>      stores result in cfun */
>>>>   unsigned int incoming_stack_boundary
>>>>     = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary
>>>>        ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary);
>>>>   unsigned int stack_realign
>>>>     = (incoming_stack_boundary
>>>>        < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor
>>>>           ? crtl->max_used_stack_slot_alignment
>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>> We call it after RA when all spill slots are allocated and check if we
>>> may relax stack alignment. Don't see any problem here.
>>
>> Please see
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69454#c26
>>
>> Why did LRA crash then?
>
> Because it tries a patch [1] which doesn't fix stack alignment and STV
> enabling and therefore doesn't resolve the problem when
> -mpreferred-stack-boundary=2 is used.
>

No, it is because RA doesn't increase stack alignment.  You have to
have the correct stack alignment requirement before entering RA.


-- 
H.J.

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

[Ilya Enkovich]

2016-02-02 17:03 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
> On Tue, Feb 2, 2016 at 5:55 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2016-02-02 16:25 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>> On Tue, Feb 2, 2016 at 5:21 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>> 2016-02-02 16:14 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>> On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>> 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>>>> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>>>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>>>>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote:
>>>>>>>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> >> 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.
>>>>>>>>>>>> >
>>>>>>>>>>>> > Uros, any preferences here?  I mean, it is possible to use
>>>>>>>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment
>>>>>>>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2
>>>>>>>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do
>>>>>>>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets
>>>>>>>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...)
>>>>>>>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment.
>>>>>>>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway,
>>>>>>>>>>>> > and that is something we don't want to affect.
>>>>>>>>>>>>
>>>>>>>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV
>>>>>>>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for
>>>>>>>>>>>> explicit -mstv in case stack boundary requirement is not satisfied.
>>>>>>>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can
>>>>>>>>>>>> revisit this decision for later gcc versions.
>>>>>>>>>>>>
>>>>>>>>>>>> I think disabling STV is less surprising option than increasing stack
>>>>>>>>>>>> boundary behind the user's back.
>>>>>>>>>>>
>>>>>>>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html
>>>>>>>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)?
>>>>>>>>>>> I believe it does just that.
>>>>>>>>>>
>>>>>>>>>> This patch is WRONG.
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> H.J.
>>>>>>>>>
>>>>>>>>> You will run into the same ICE with
>>>>>>>>>
>>>>>>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>>>>>>>
>>>>>>>>> in a leaf function which needs DImode spill/fill.
>>>>>>>>
>>>>>>>> Why would we need DImode spill/fill having no DImode registers?
>>>>>>>>
>>>>>>>
>>>>>>> Because STV is enabled with
>>>>>>>
>>>>>>>  -mincoming-stack-boundary=2 -msse2 -O2 -m32
>>>>>>
>>>>>> I misread it as -mpreferred-... So why would we fail having a proper
>>>>>> preferred stack alignment? AFAIK leaf function doesn't affect
>>>>>> alignment until we finalize it after RA.
>>>>>>
>>>>>
>>>>> /* Finalize stack_realign_needed flag, which will guide prologue/epilogue
>>>>>    to be generated in correct form.  */
>>>>> static void
>>>>> ix86_finalize_stack_realign_flags (void)
>>>>> {
>>>>>   /* Check if stack realign is really needed after reload, and
>>>>>      stores result in cfun */
>>>>>   unsigned int incoming_stack_boundary
>>>>>     = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary
>>>>>        ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary);
>>>>>   unsigned int stack_realign
>>>>>     = (incoming_stack_boundary
>>>>>        < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor
>>>>>           ? crtl->max_used_stack_slot_alignment
>>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>
>>>> We call it after RA when all spill slots are allocated and check if we
>>>> may relax stack alignment. Don't see any problem here.
>>>
>>> Please see
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69454#c26
>>>
>>> Why did LRA crash then?
>>
>> Because it tries a patch [1] which doesn't fix stack alignment and STV
>> enabling and therefore doesn't resolve the problem when
>> -mpreferred-stack-boundary=2 is used.
>>
>
> No, it is because RA doesn't increase stack alignment.  You have to
> have the correct stack alignment requirement before entering RA.

And it's too late to do it after STV pass and therefore we disable it
when stack is not properly aligned. I think this argumentation goes in
a loop.

Thanks,
Ilya

>
>
> --
> H.J.

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

[Jakub Jelinek]

Hi!

On Tue, Feb 02, 2016 at 05:09:34PM +0300, Ilya Enkovich wrote:
> And it's too late to do it after STV pass and therefore we disable it
> when stack is not properly aligned. I think this argumentation goes in
> a loop.

This is a P1 that needs to be fixed, so that we don't defer this forever,
what about the following patch?  As neither stv nor preferred-stack-boundary
nor incoming-stack-boundary are allowed target attribute/GCC target pragma
switches, I can't find a problem with that.  We don't track at expansion
time whether the function is leaf or not, so the patch pessimistically
assumes that the function might be leaf and check both incoming and
preferred stack boundaries.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-02-03  Jakub Jelinek  <jakub@redhat.com>
	    Ilya Enkovich  <enkovich.gnu@gmail.com>
	    H.J. Lu  <hongjiu.lu@intel.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
	might not be aligned enough.
	(ix86_minimum_alignment): Assert that TARGET_STV is false.

	* gcc.target/i386/pr69454-1.c: New test.
	* gcc.target/i386/pr69454-2.c: New test.

--- gcc/config/i386/i386.c.jj	2016-02-02 20:42:01.024287587 +0100
+++ gcc/config/i386/i386.c	2016-02-03 18:45:43.271997909 +0100
@@ -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,13 @@ ix86_option_override_internal (bool main
     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 or
+     -mincoming-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
+      || ix86_incoming_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;
@@ -29323,7 +29320,10 @@ ix86_minimum_alignment (tree exp, machin
   if ((mode == DImode || (type && TYPE_MODE (type) == DImode))
       && (!type || !TYPE_USER_ALIGN (type))
       && (!decl || !DECL_USER_ALIGN (decl)))
-    return 32;
+    {
+      gcc_checking_assert (!TARGET_STV);
+      return 32;
+    }
 
   return align;
 }
--- gcc/testsuite/gcc.target/i386/pr69454-1.c.jj	2016-02-03 18:44:17.642175753 +0100
+++ gcc/testsuite/gcc.target/i386/pr69454-1.c	2016-02-03 18:44:17.642175753 +0100
@@ -0,0 +1,11 @@
+/* { dg-do compile { target { ia32 } } } */
+/* { dg-options "-O2 -msse2 -mno-accumulate-outgoing-args -mpreferred-stack-boundary=2" } */
+
+typedef struct { long long w64[2]; } V128;
+extern V128* fn2(void);
+long long a;
+V128 b;
+void fn1() {
+  V128 *c = fn2();
+  c->w64[0] = a ^ b.w64[0];
+}
--- gcc/testsuite/gcc.target/i386/pr69454-2.c.jj	2016-02-03 18:44:17.655175574 +0100
+++ gcc/testsuite/gcc.target/i386/pr69454-2.c	2016-02-03 18:44:17.655175574 +0100
@@ -0,0 +1,13 @@
+/* { dg-do compile { target { ia32 } } } */
+/* { dg-options "-O2 -mpreferred-stack-boundary=2" } */
+
+extern void fn2 ();
+long long a, b;
+
+void fn1 ()
+{
+  long long c = a;
+  a = b ^ a;
+  fn2 ();
+  a = c;
+}


	Jakub

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

[Uros Bizjak]

On Wed, Feb 3, 2016 at 9:11 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> On Tue, Feb 02, 2016 at 05:09:34PM +0300, Ilya Enkovich wrote:
>> And it's too late to do it after STV pass and therefore we disable it
>> when stack is not properly aligned. I think this argumentation goes in
>> a loop.
>
> This is a P1 that needs to be fixed, so that we don't defer this forever,
> what about the following patch?  As neither stv nor preferred-stack-boundary
> nor incoming-stack-boundary are allowed target attribute/GCC target pragma
> switches, I can't find a problem with that.  We don't track at expansion
> time whether the function is leaf or not, so the patch pessimistically
> assumes that the function might be leaf and check both incoming and
> preferred stack boundaries.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2016-02-03  Jakub Jelinek  <jakub@redhat.com>
>             Ilya Enkovich  <enkovich.gnu@gmail.com>
>             H.J. Lu  <hongjiu.lu@intel.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
>         might not be aligned enough.
>         (ix86_minimum_alignment): Assert that TARGET_STV is false.
>
>         * gcc.target/i386/pr69454-1.c: New test.
>         * gcc.target/i386/pr69454-2.c: New test.

OK.

We can eventually introduce realignment for STV for gcc-7.

Thanks,
Uros.

> --- gcc/config/i386/i386.c.jj   2016-02-02 20:42:01.024287587 +0100
> +++ gcc/config/i386/i386.c      2016-02-03 18:45:43.271997909 +0100
> @@ -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,13 @@ ix86_option_override_internal (bool main
>      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 or
> +     -mincoming-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
> +      || ix86_incoming_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;
> @@ -29323,7 +29320,10 @@ ix86_minimum_alignment (tree exp, machin
>    if ((mode == DImode || (type && TYPE_MODE (type) == DImode))
>        && (!type || !TYPE_USER_ALIGN (type))
>        && (!decl || !DECL_USER_ALIGN (decl)))
> -    return 32;
> +    {
> +      gcc_checking_assert (!TARGET_STV);
> +      return 32;
> +    }
>
>    return align;
>  }
> --- gcc/testsuite/gcc.target/i386/pr69454-1.c.jj        2016-02-03 18:44:17.642175753 +0100
> +++ gcc/testsuite/gcc.target/i386/pr69454-1.c   2016-02-03 18:44:17.642175753 +0100
> @@ -0,0 +1,11 @@
> +/* { dg-do compile { target { ia32 } } } */
> +/* { dg-options "-O2 -msse2 -mno-accumulate-outgoing-args -mpreferred-stack-boundary=2" } */
> +
> +typedef struct { long long w64[2]; } V128;
> +extern V128* fn2(void);
> +long long a;
> +V128 b;
> +void fn1() {
> +  V128 *c = fn2();
> +  c->w64[0] = a ^ b.w64[0];
> +}
> --- gcc/testsuite/gcc.target/i386/pr69454-2.c.jj        2016-02-03 18:44:17.655175574 +0100
> +++ gcc/testsuite/gcc.target/i386/pr69454-2.c   2016-02-03 18:44:17.655175574 +0100
> @@ -0,0 +1,13 @@
> +/* { dg-do compile { target { ia32 } } } */
> +/* { dg-options "-O2 -mpreferred-stack-boundary=2" } */
> +
> +extern void fn2 ();
> +long long a, b;
> +
> +void fn1 ()
> +{
> +  long long c = a;
> +  a = b ^ a;
> +  fn2 ();
> +  a = c;
> +}
>
>
>         Jakub

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

[H.J. Lu]

On Thu, Feb 4, 2016 at 12:02 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Wed, Feb 3, 2016 at 9:11 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> Hi!
>>
>> On Tue, Feb 02, 2016 at 05:09:34PM +0300, Ilya Enkovich wrote:
>>> And it's too late to do it after STV pass and therefore we disable it
>>> when stack is not properly aligned. I think this argumentation goes in
>>> a loop.
>>
>> This is a P1 that needs to be fixed, so that we don't defer this forever,
>> what about the following patch?  As neither stv nor preferred-stack-boundary
>> nor incoming-stack-boundary are allowed target attribute/GCC target pragma
>> switches, I can't find a problem with that.  We don't track at expansion
>> time whether the function is leaf or not, so the patch pessimistically
>> assumes that the function might be leaf and check both incoming and
>> preferred stack boundaries.
>>
>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>
>> 2016-02-03  Jakub Jelinek  <jakub@redhat.com>
>>             Ilya Enkovich  <enkovich.gnu@gmail.com>
>>             H.J. Lu  <hongjiu.lu@intel.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
>>         might not be aligned enough.
>>         (ix86_minimum_alignment): Assert that TARGET_STV is false.
>>
>>         * gcc.target/i386/pr69454-1.c: New test.
>>         * gcc.target/i386/pr69454-2.c: New test.
>
> OK.
>
> We can eventually introduce realignment for STV for gcc-7.
>
> Thanks,
> Uros.
>
>> --- gcc/config/i386/i386.c.jj   2016-02-02 20:42:01.024287587 +0100
>> +++ gcc/config/i386/i386.c      2016-02-03 18:45:43.271997909 +0100
>> @@ -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,13 @@ ix86_option_override_internal (bool main
>>      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 or
>> +     -mincoming-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
>> +      || ix86_incoming_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;
>> @@ -29323,7 +29320,10 @@ ix86_minimum_alignment (tree exp, machin
>>    if ((mode == DImode || (type && TYPE_MODE (type) == DImode))
>>        && (!type || !TYPE_USER_ALIGN (type))
>>        && (!decl || !DECL_USER_ALIGN (decl)))
>> -    return 32;
>> +    {
>> +      gcc_checking_assert (!TARGET_STV);
>> +      return 32;
>> +    }
>>
>>    return align;
>>  }
>> --- gcc/testsuite/gcc.target/i386/pr69454-1.c.jj        2016-02-03 18:44:17.642175753 +0100
>> +++ gcc/testsuite/gcc.target/i386/pr69454-1.c   2016-02-03 18:44:17.642175753 +0100
>> @@ -0,0 +1,11 @@
>> +/* { dg-do compile { target { ia32 } } } */
>> +/* { dg-options "-O2 -msse2 -mno-accumulate-outgoing-args -mpreferred-stack-boundary=2" } */
>> +
>> +typedef struct { long long w64[2]; } V128;
>> +extern V128* fn2(void);
>> +long long a;
>> +V128 b;
>> +void fn1() {
>> +  V128 *c = fn2();
>> +  c->w64[0] = a ^ b.w64[0];
>> +}
>> --- gcc/testsuite/gcc.target/i386/pr69454-2.c.jj        2016-02-03 18:44:17.655175574 +0100
>> +++ gcc/testsuite/gcc.target/i386/pr69454-2.c   2016-02-03 18:44:17.655175574 +0100
>> @@ -0,0 +1,13 @@
>> +/* { dg-do compile { target { ia32 } } } */
>> +/* { dg-options "-O2 -mpreferred-stack-boundary=2" } */
>> +
>> +extern void fn2 ();
>> +long long a, b;
>> +
>> +void fn1 ()
>> +{
>> +  long long c = a;
>> +  a = b ^ a;
>> +  fn2 ();
>> +  a = c;
>> +}
>>
>>
>>         Jakub

This caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69677

-- 
H.J.