* PATCH: PR middle-end/48608: Alignment adjust of local variables is lost @ 2011-04-14 13:34 H.J. Lu 2011-04-14 13:57 ` Richard Guenther 0 siblings, 1 reply; 10+ messages in thread From: H.J. Lu @ 2011-04-14 13:34 UTC (permalink / raw) To: gcc-patches 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; } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost 2011-04-14 13:34 PATCH: PR middle-end/48608: Alignment adjust of local variables is lost H.J. Lu @ 2011-04-14 13:57 ` Richard Guenther 2011-04-14 14:01 ` H.J. Lu 0 siblings, 1 reply; 10+ messages in thread From: Richard Guenther @ 2011-04-14 13:57 UTC (permalink / raw) To: H.J. Lu; +Cc: H.J. Lu, gcc-patches 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; > } > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost 2011-04-14 13:57 ` Richard Guenther @ 2011-04-14 14:01 ` H.J. Lu 2011-04-14 14:09 ` Richard Guenther 0 siblings, 1 reply; 10+ messages in thread From: H.J. Lu @ 2011-04-14 14:01 UTC (permalink / raw) To: Richard Guenther; +Cc: GCC Patches 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost 2011-04-14 14:01 ` H.J. Lu @ 2011-04-14 14:09 ` Richard Guenther 2011-04-14 14:27 ` H.J. Lu 0 siblings, 1 reply; 10+ messages in thread From: Richard Guenther @ 2011-04-14 14:09 UTC (permalink / raw) To: H.J. Lu; +Cc: GCC Patches 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. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost 2011-04-14 14:09 ` Richard Guenther @ 2011-04-14 14:27 ` H.J. Lu 2011-04-14 14:30 ` Richard Guenther 0 siblings, 1 reply; 10+ messages in thread From: H.J. Lu @ 2011-04-14 14:27 UTC (permalink / raw) To: Richard Guenther; +Cc: GCC Patches 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); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost 2011-04-14 14:27 ` H.J. Lu @ 2011-04-14 14:30 ` Richard Guenther 2011-04-14 15:26 ` Michael Matz 0 siblings, 1 reply; 10+ messages in thread From: Richard Guenther @ 2011-04-14 14:30 UTC (permalink / raw) To: H.J. Lu; +Cc: GCC Patches 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); > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost 2011-04-14 14:30 ` Richard Guenther @ 2011-04-14 15:26 ` Michael Matz 2011-04-14 15:48 ` H.J. Lu 0 siblings, 1 reply; 10+ messages in thread From: Michael Matz @ 2011-04-14 15:26 UTC (permalink / raw) To: Richard Guenther; +Cc: H.J. Lu, GCC Patches [-- 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost 2011-04-14 15:26 ` Michael Matz @ 2011-04-14 15:48 ` H.J. Lu 2011-04-14 15:57 ` Richard Guenther 2011-04-15 12:34 ` Michael Matz 0 siblings, 2 replies; 10+ messages in thread From: H.J. Lu @ 2011-04-14 15:48 UTC (permalink / raw) To: Michael Matz; +Cc: Richard Guenther, GCC Patches 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost 2011-04-14 15:48 ` H.J. Lu @ 2011-04-14 15:57 ` Richard Guenther 2011-04-15 12:34 ` Michael Matz 1 sibling, 0 replies; 10+ messages in thread From: Richard Guenther @ 2011-04-14 15:57 UTC (permalink / raw) To: H.J. Lu; +Cc: Michael Matz, GCC Patches 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. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PATCH: PR middle-end/48608: Alignment adjust of local variables is lost 2011-04-14 15:48 ` H.J. Lu 2011-04-14 15:57 ` Richard Guenther @ 2011-04-15 12:34 ` Michael Matz 1 sibling, 0 replies; 10+ messages in thread From: Michael Matz @ 2011-04-15 12:34 UTC (permalink / raw) To: H.J. Lu; +Cc: Richard Guenther, GCC Patches [-- 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-04-15 11:52 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-04-14 13:34 PATCH: PR middle-end/48608: Alignment adjust of local variables is lost H.J. Lu 2011-04-14 13:57 ` Richard Guenther 2011-04-14 14:01 ` H.J. Lu 2011-04-14 14:09 ` Richard Guenther 2011-04-14 14:27 ` H.J. Lu 2011-04-14 14:30 ` Richard Guenther 2011-04-14 15:26 ` Michael Matz 2011-04-14 15:48 ` H.J. Lu 2011-04-14 15:57 ` Richard Guenther 2011-04-15 12:34 ` Michael Matz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).