Respect user align for local variable

Message ID 20200330103127.7073-1-kito.cheng@sifive.com
State New
Headers show
Series
  • Respect user align for local variable
Related show

Commit Message

Kito Cheng March 30, 2020, 10:31 a.m.
gcc/ChangeLog

	* cfgexpand.c (align_local_variable): Check DECL_USER_ALIGN.
---
 gcc/cfgexpand.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

-- 
2.25.2

Comments

Iain Buclaw via Gcc-patches March 30, 2020, 11:20 a.m. | #1
On Mon, Mar 30, 2020 at 06:31:27PM +0800, Kito Cheng wrote:
> gcc/ChangeLog

> 

> 	* cfgexpand.c (align_local_variable): Check DECL_USER_ALIGN.


Why?  LOCAL_DECL_ALIGNMENT surely shouldn't decrease alignment of decls
with DECL_USER_ALIGN vars (but do you have evidence that it does),
but it is just fine to increase alignment of vars with explicit alignment
requirements if the compiler thinks it is beneficial.  Larger alignment
still satisfies the user requirement...

	Jakub
Kito Cheng March 30, 2020, 12:37 p.m. | #2
Hi Jakub:

Thanks for your correction, I read the doc for the aligned attribute
again[1], it's minimum alignment not restricted alignment, I thought
it should honor to the alignment attribute, Richard Biener
suggested[2] should put an assertion here to make sure never decrease
alignment here, so I'll send another patch for add assertion.

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes
[2] https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542808.html

On Mon, Mar 30, 2020 at 7:21 PM Jakub Jelinek <jakub@redhat.com> wrote:
>

> On Mon, Mar 30, 2020 at 06:31:27PM +0800, Kito Cheng wrote:

> > gcc/ChangeLog

> >

> >       * cfgexpand.c (align_local_variable): Check DECL_USER_ALIGN.

>

> Why?  LOCAL_DECL_ALIGNMENT surely shouldn't decrease alignment of decls

> with DECL_USER_ALIGN vars (but do you have evidence that it does),

> but it is just fine to increase alignment of vars with explicit alignment

> requirements if the compiler thinks it is beneficial.  Larger alignment

> still satisfies the user requirement...

>

>         Jakub

>

Patch

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index a7ec77d5c85..19a020b4b97 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -369,13 +369,19 @@  align_local_variable (tree decl, bool really_expand)
     align = TYPE_ALIGN (TREE_TYPE (decl));
   else
     {
-      align = LOCAL_DECL_ALIGNMENT (decl);
-      /* Don't change DECL_ALIGN when called from estimated_stack_frame_size.
-	 That is done before IPA and could bump alignment based on host
-	 backend even for offloaded code which wants different
-	 LOCAL_DECL_ALIGNMENT.  */
-      if (really_expand)
-	SET_DECL_ALIGN (decl, align);
+      if (DECL_USER_ALIGN (decl))
+	align = DECL_ALIGN (decl);
+      else
+	{
+	  align = LOCAL_DECL_ALIGNMENT (decl);
+	  /* Don't change DECL_ALIGN when called from
+	     estimated_stack_frame_size.
+	     That is done before IPA and could bump alignment based on host
+	     backend even for offloaded code which wants different
+	     LOCAL_DECL_ALIGNMENT.  */
+	  if (really_expand)
+	    SET_DECL_ALIGN (decl, align);
+	}
     }
   return align / BITS_PER_UNIT;
 }