diff mbox

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

Message ID BANLkTinqEzz9iyzJXvSqOWhZYoQec_RpKg@mail.gmail.com
State New
Headers show

Commit Message

H.J. Lu April 14, 2011, 2:27 p.m. UTC 
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.

Comments

Richard Biener April 14, 2011, 2:30 p.m. UTC | #1 
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);
>
Michael Matz April 14, 2011, 3:26 p.m. UTC | #2 
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.
H.J. Lu April 14, 2011, 3:48 p.m. UTC | #3 
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.
Richard Biener April 14, 2011, 3:57 p.m. UTC | #4 
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.
>
Michael Matz April 15, 2011, 11:52 a.m. UTC | #5 
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.
diff mbox

Patch

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