PATCH: PR middle-end/48608: Alignment adjust of local variables is lost

PATCH: PR middle-end/48608: Alignment adjust of local variables is lost

[H.J. Lu]

We have

static unsigned int
get_decl_align_unit (tree decl)
{
  unsigned int align = LOCAL_DECL_ALIGNMENT (decl);
  return align / BITS_PER_UNIT;
}

LOCAL_DECL_ALIGNMENT may increase alignment for local variable.  But it is
never saved.  DECL_ALIGN (decl) returns the old alignment.  This patch
updates DECL_ALIGN if needed.  OK for trunk if there are no regressions?

Thanks.

H.J.
---
2011-04-14  H.J. Lu  <hongjiu.lu@intel.com>

	PR middle-end/48608
	* cfgexpand.c (get_decl_align_unit): Update DECL_ALIGN if needed.

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index cc1382f..e79d50c 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -212,6 +212,8 @@ static unsigned int
 get_decl_align_unit (tree decl)
 {
   unsigned int align = LOCAL_DECL_ALIGNMENT (decl);
+  if (align > DECL_ALIGN (decl))
+    DECL_ALIGN (decl) = align;
   return align / BITS_PER_UNIT;
 }
 

Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost

[Richard Guenther]

On Thu, Apr 14, 2011 at 3:34 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
> We have
>
> static unsigned int
> get_decl_align_unit (tree decl)
> {
>  unsigned int align = LOCAL_DECL_ALIGNMENT (decl);
>  return align / BITS_PER_UNIT;
> }
>
> LOCAL_DECL_ALIGNMENT may increase alignment for local variable.  But it is
> never saved.  DECL_ALIGN (decl) returns the old alignment.  This patch
> updates DECL_ALIGN if needed.  OK for trunk if there are no regressions?

A get_* function does not seem like a good place to do such things.
Why does it matter that DECL_ALIGN is updated?

> Thanks.
>
> H.J.
> ---
> 2011-04-14  H.J. Lu  <hongjiu.lu@intel.com>
>
>        PR middle-end/48608
>        * cfgexpand.c (get_decl_align_unit): Update DECL_ALIGN if needed.
>
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index cc1382f..e79d50c 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -212,6 +212,8 @@ static unsigned int
>  get_decl_align_unit (tree decl)
>  {
>   unsigned int align = LOCAL_DECL_ALIGNMENT (decl);
> +  if (align > DECL_ALIGN (decl))
> +    DECL_ALIGN (decl) = align;
>   return align / BITS_PER_UNIT;
>  }
>
>

Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost

[H.J. Lu]

On Thu, Apr 14, 2011 at 6:57 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Thu, Apr 14, 2011 at 3:34 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>> We have
>>
>> static unsigned int
>> get_decl_align_unit (tree decl)
>> {
>>  unsigned int align = LOCAL_DECL_ALIGNMENT (decl);
>>  return align / BITS_PER_UNIT;
>> }
>>
>> LOCAL_DECL_ALIGNMENT may increase alignment for local variable.  But it is
>> never saved.  DECL_ALIGN (decl) returns the old alignment.  This patch
>> updates DECL_ALIGN if needed.  OK for trunk if there are no regressions?
>
> A get_* function does not seem like a good place to do such things.

Any suggestion to how to do it properly? I can rename
get_decl_align_unit to align_local_variable.

> Why does it matter that DECL_ALIGN is updated?
>

My port needs accurate alignment information on local variables.

-- 
H.J.

Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost

[Richard Guenther]

On Thu, Apr 14, 2011 at 4:01 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Apr 14, 2011 at 6:57 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Thu, Apr 14, 2011 at 3:34 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>>> We have
>>>
>>> static unsigned int
>>> get_decl_align_unit (tree decl)
>>> {
>>>  unsigned int align = LOCAL_DECL_ALIGNMENT (decl);
>>>  return align / BITS_PER_UNIT;
>>> }
>>>
>>> LOCAL_DECL_ALIGNMENT may increase alignment for local variable.  But it is
>>> never saved.  DECL_ALIGN (decl) returns the old alignment.  This patch
>>> updates DECL_ALIGN if needed.  OK for trunk if there are no regressions?
>>
>> A get_* function does not seem like a good place to do such things.
>
> Any suggestion to how to do it properly? I can rename
> get_decl_align_unit to align_local_variable.

That works for me.

>> Why does it matter that DECL_ALIGN is updated?
>>
>
> My port needs accurate alignment information on local variables.

I see.

Richard.

> --
> H.J.
>

Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost

[H.J. Lu]

On Thu, Apr 14, 2011 at 7:09 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Thu, Apr 14, 2011 at 4:01 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Apr 14, 2011 at 6:57 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Thu, Apr 14, 2011 at 3:34 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>>>> We have
>>>>
>>>> static unsigned int
>>>> get_decl_align_unit (tree decl)
>>>> {
>>>>  unsigned int align = LOCAL_DECL_ALIGNMENT (decl);
>>>>  return align / BITS_PER_UNIT;
>>>> }
>>>>
>>>> LOCAL_DECL_ALIGNMENT may increase alignment for local variable.  But it is
>>>> never saved.  DECL_ALIGN (decl) returns the old alignment.  This patch
>>>> updates DECL_ALIGN if needed.  OK for trunk if there are no regressions?
>>>
>>> A get_* function does not seem like a good place to do such things.
>>
>> Any suggestion to how to do it properly? I can rename
>> get_decl_align_unit to align_local_variable.
>
> That works for me.
>
>>> Why does it matter that DECL_ALIGN is updated?
>>>
>>
>> My port needs accurate alignment information on local variables.
>
> I see.

Here is the updated patch.  OK for trunk if there are no regressions?

Thanks.

-- 
H.J.
---
2011-04-14  H.J. Lu  <hongjiu.lu@intel.com>

	PR middle-end/48608
	* cfgexpand.c (get_decl_align_unit): Renamed to ...
	(align_local_variable): This.  Update DECL_ALIGN if needed.
	(add_stack_var): Updated.
	(expand_one_stack_var): Likewise.

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index cc1382f..d38d2f9 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -205,13 +205,15 @@ static bool has_protected_decls;
    smaller than our cutoff threshold.  Used for -Wstack-protector.  */
 static bool has_short_buffer;

-/* Discover the byte alignment to use for DECL.  Ignore alignment
+/* Compute the byte alignment to use for DECL.  Ignore alignment
    we can't do with expected alignment of the stack boundary.  */

 static unsigned int
-get_decl_align_unit (tree decl)
+align_local_variable (tree decl)
 {
   unsigned int align = LOCAL_DECL_ALIGNMENT (decl);
+  if (align > DECL_ALIGN (decl))
+    DECL_ALIGN (decl) = align;
   return align / BITS_PER_UNIT;
 }

@@ -273,7 +275,7 @@ add_stack_var (tree decl)
      variables that are simultaneously live.  */
   if (v->size == 0)
     v->size = 1;
-  v->alignb = get_decl_align_unit (SSAVAR (decl));
+  v->alignb = align_local_variable (SSAVAR (decl));

   /* All variables are initially in their own partition.  */
   v->representative = stack_vars_num;
@@ -905,7 +907,7 @@ expand_one_stack_var (tree var)
   unsigned byte_align;

   size = tree_low_cst (DECL_SIZE_UNIT (SSAVAR (var)), 1);
-  byte_align = get_decl_align_unit (SSAVAR (var));
+  byte_align = align_local_variable (SSAVAR (var));

   /* We handle highly aligned variables in expand_stack_vars.  */
   gcc_assert (byte_align * BITS_PER_UNIT <= MAX_SUPPORTED_STACK_ALIGNMENT);

Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost

[Richard Guenther]

On Thu, Apr 14, 2011 at 4:27 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Apr 14, 2011 at 7:09 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Thu, Apr 14, 2011 at 4:01 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Thu, Apr 14, 2011 at 6:57 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Thu, Apr 14, 2011 at 3:34 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>>>>> We have
>>>>>
>>>>> static unsigned int
>>>>> get_decl_align_unit (tree decl)
>>>>> {
>>>>>  unsigned int align = LOCAL_DECL_ALIGNMENT (decl);
>>>>>  return align / BITS_PER_UNIT;
>>>>> }
>>>>>
>>>>> LOCAL_DECL_ALIGNMENT may increase alignment for local variable.  But it is
>>>>> never saved.  DECL_ALIGN (decl) returns the old alignment.  This patch
>>>>> updates DECL_ALIGN if needed.  OK for trunk if there are no regressions?
>>>>
>>>> A get_* function does not seem like a good place to do such things.
>>>
>>> Any suggestion to how to do it properly? I can rename
>>> get_decl_align_unit to align_local_variable.
>>
>> That works for me.
>>
>>>> Why does it matter that DECL_ALIGN is updated?
>>>>
>>>
>>> My port needs accurate alignment information on local variables.
>>
>> I see.
>
> Here is the updated patch.  OK for trunk if there are no regressions?
>
> Thanks.
>
> --
> H.J.
> ---
> 2011-04-14  H.J. Lu  <hongjiu.lu@intel.com>
>
>        PR middle-end/48608
>        * cfgexpand.c (get_decl_align_unit): Renamed to ...
>        (align_local_variable): This.  Update DECL_ALIGN if needed.
>        (add_stack_var): Updated.
>        (expand_one_stack_var): Likewise.
>
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index cc1382f..d38d2f9 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -205,13 +205,15 @@ static bool has_protected_decls;
>    smaller than our cutoff threshold.  Used for -Wstack-protector.  */
>  static bool has_short_buffer;
>
> -/* Discover the byte alignment to use for DECL.  Ignore alignment
> +/* Compute the byte alignment to use for DECL.  Ignore alignment
>    we can't do with expected alignment of the stack boundary.  */
>
>  static unsigned int
> -get_decl_align_unit (tree decl)
> +align_local_variable (tree decl)
>  {
>   unsigned int align = LOCAL_DECL_ALIGNMENT (decl);
> +  if (align > DECL_ALIGN (decl))
> +    DECL_ALIGN (decl) = align;

Shouldn't this unconditionally set DECL_ALIGN in case
LOCAL_DECL_ALINGMENT returns something smaller?

Ok with that change.

Thanks,
Richard.

>   return align / BITS_PER_UNIT;
>  }
>
> @@ -273,7 +275,7 @@ add_stack_var (tree decl)
>      variables that are simultaneously live.  */
>   if (v->size == 0)
>     v->size = 1;
> -  v->alignb = get_decl_align_unit (SSAVAR (decl));
> +  v->alignb = align_local_variable (SSAVAR (decl));
>
>   /* All variables are initially in their own partition.  */
>   v->representative = stack_vars_num;
> @@ -905,7 +907,7 @@ expand_one_stack_var (tree var)
>   unsigned byte_align;
>
>   size = tree_low_cst (DECL_SIZE_UNIT (SSAVAR (var)), 1);
> -  byte_align = get_decl_align_unit (SSAVAR (var));
> +  byte_align = align_local_variable (SSAVAR (var));
>
>   /* We handle highly aligned variables in expand_stack_vars.  */
>   gcc_assert (byte_align * BITS_PER_UNIT <= MAX_SUPPORTED_STACK_ALIGNMENT);
>

Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost

[Michael Matz]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 595 bytes --]

Hi,

On Thu, 14 Apr 2011, Richard Guenther wrote:

> > +  if (align > DECL_ALIGN (decl))
> > +    DECL_ALIGN (decl) = align;
> 
> Shouldn't this unconditionally set DECL_ALIGN in case
> LOCAL_DECL_ALINGMENT returns something smaller?

Decreasing alignment of DECLs points to a problem elsewhere, so perhaps an 
assert that this doesn't happen is better.  Decreasing is a problem 
because it's not conservative: there might have been code generated 
already assuming the once larger alignment that then possibly breaks if it 
turns out the alignment is actually smaller.


Ciao,
Michael.

Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost

[H.J. Lu]

On Thu, Apr 14, 2011 at 8:26 AM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Thu, 14 Apr 2011, Richard Guenther wrote:
>
>> > +  if (align > DECL_ALIGN (decl))
>> > +    DECL_ALIGN (decl) = align;
>>
>> Shouldn't this unconditionally set DECL_ALIGN in case
>> LOCAL_DECL_ALINGMENT returns something smaller?
>
> Decreasing alignment of DECLs points to a problem elsewhere, so perhaps an
> assert that this doesn't happen is better.  Decreasing is a problem
> because it's not conservative: there might have been code generated
> already assuming the once larger alignment that then possibly breaks if it
> turns out the alignment is actually smaller.

ia32 may decrease local variable alignment:

  /* Don't do dynamic stack realignment for long long objects with
     -mpreferred-stack-boundary=2.  */
  if (!TARGET_64BIT
      && align == 64
      && ix86_preferred_stack_boundary < 64
      && (mode == DImode || (type && TYPE_MODE (type) == DImode))
      && (!type || !TYPE_USER_ALIGN (type))
      && (!decl || !DECL_USER_ALIGN (decl)))
    align = 32;

I am running bootstrap/test on Linux/x86-64 and Linux/ia32.

-- 
H.J.

Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost

[Richard Guenther]

On Thu, Apr 14, 2011 at 5:48 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Apr 14, 2011 at 8:26 AM, Michael Matz <matz@suse.de> wrote:
>> Hi,
>>
>> On Thu, 14 Apr 2011, Richard Guenther wrote:
>>
>>> > +  if (align > DECL_ALIGN (decl))
>>> > +    DECL_ALIGN (decl) = align;
>>>
>>> Shouldn't this unconditionally set DECL_ALIGN in case
>>> LOCAL_DECL_ALINGMENT returns something smaller?
>>
>> Decreasing alignment of DECLs points to a problem elsewhere, so perhaps an
>> assert that this doesn't happen is better.  Decreasing is a problem
>> because it's not conservative: there might have been code generated
>> already assuming the once larger alignment that then possibly breaks if it
>> turns out the alignment is actually smaller.
>
> ia32 may decrease local variable alignment:
>
>  /* Don't do dynamic stack realignment for long long objects with
>     -mpreferred-stack-boundary=2.  */
>  if (!TARGET_64BIT
>      && align == 64
>      && ix86_preferred_stack_boundary < 64
>      && (mode == DImode || (type && TYPE_MODE (type) == DImode))
>      && (!type || !TYPE_USER_ALIGN (type))
>      && (!decl || !DECL_USER_ALIGN (decl)))
>    align = 32;
>
> I am running bootstrap/test on Linux/x86-64 and Linux/ia32.

That's broken.  It may cause explicit alignment checks to be bogously
optimized away.

Richard.

> --
> H.J.
>

Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost

[Michael Matz]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1193 bytes --]

Hi,

On Thu, 14 Apr 2011, H.J. Lu wrote:

> >> > +  if (align > DECL_ALIGN (decl))
> >> > +    DECL_ALIGN (decl) = align;
> >>
> >> Shouldn't this unconditionally set DECL_ALIGN in case
> >> LOCAL_DECL_ALINGMENT returns something smaller?
> >
> > Decreasing alignment of DECLs points to a problem elsewhere, so perhaps an
> > assert that this doesn't happen is better.  Decreasing is a problem
> > because it's not conservative: there might have been code generated
> > already assuming the once larger alignment that then possibly breaks if it
> > turns out the alignment is actually smaller.
> 
> ia32 may decrease local variable alignment:
> 
>   /* Don't do dynamic stack realignment for long long objects with
>      -mpreferred-stack-boundary=2.  */
>   if (!TARGET_64BIT
>       && align == 64
>       && ix86_preferred_stack_boundary < 64
>       && (mode == DImode || (type && TYPE_MODE (type) == DImode))
>       && (!type || !TYPE_USER_ALIGN (type))
>       && (!decl || !DECL_USER_ALIGN (decl)))
>     align = 32;

But it hopefully does so before gimplification?  I.e. before real code is 
generated that could possibly make use of the large alignment?


Ciao,
Michael.