[PATCH v3] Fix alignment for local variable [PR90811]

[PATCH v3] Fix alignment for local variable [PR90811]

[Kito Cheng]

 - The alignment for local variable was adjust during estimate_stack_frame_size,
   however it seems wrong spot to adjust that, expand phase will adjust that
   but it little too late to some gimple optimization, which rely on certain
   target hooks need to check alignment, forwprop is an example for
   that, result of simplify_builtin_call rely on the alignment on some
   target like ARM or RISC-V.

 - This patch fix gfortran.dg/pr45636.f90 for arm and riscv.

 - Regression test on riscv32/riscv64 and x86_64-linux-gnu, no new fail
   introduced.

gcc/ChangeLog

	PR target/90811
	* Makefile.in (OBJS): Add tree-adjust-alignment.o.
	* tree-adjust-alignment.cc (pass_data_adjust_alignment): New.
	(pass_adjust_alignment): New.
	(-pass_adjust_alignment::execute): New.
	(make_pass_adjust_alignment): New.
	* tree-pass.h (make_pass_adjust_alignment): New.
	* passes.def: Add pass_adjust_alignment.
---
 gcc/Makefile.in              |  1 +
 gcc/passes.def               |  1 +
 gcc/tree-adjust-alignment.cc | 88 ++++++++++++++++++++++++++++++++++++
 gcc/tree-pass.h              |  1 +
 4 files changed, 91 insertions(+)
 create mode 100644 gcc/tree-adjust-alignment.cc

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index fa9923bb270..9b73288f776 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1545,6 +1545,7 @@ OBJS = \
 	ubsan.o \
 	sanopt.o \
 	sancov.o \
+	tree-adjust-alignment.o \
 	tree-call-cdce.o \
 	tree-cfg.o \
 	tree-cfgcleanup.o \
diff --git a/gcc/passes.def b/gcc/passes.def
index 2bf2cb78fc5..92cbe587a8a 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -183,6 +183,7 @@ along with GCC; see the file COPYING3.  If not see
   NEXT_PASS (pass_oacc_device_lower);
   NEXT_PASS (pass_omp_device_lower);
   NEXT_PASS (pass_omp_target_link);
+  NEXT_PASS (pass_adjust_alignment);
   NEXT_PASS (pass_all_optimizations);
   PUSH_INSERT_PASSES_WITHIN (pass_all_optimizations)
       NEXT_PASS (pass_remove_cgraph_callee_edges);
diff --git a/gcc/tree-adjust-alignment.cc b/gcc/tree-adjust-alignment.cc
new file mode 100644
index 00000000000..1269f93e56a
--- /dev/null
+++ b/gcc/tree-adjust-alignment.cc
@@ -0,0 +1,88 @@
+/* Adjust alignment for local variable.
+   Copyright (C) 2003-2020 Free Software Foundation, Inc.
+   Contributed by Dorit Naishlos <dorit@il.ibm.com>
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "backend.h"
+#include "target.h"
+#include "tree.h"
+#include "gimple.h"
+#include "tree-pass.h"
+#include "cgraph.h"
+#include "fold-const.h"
+#include "gimple-iterator.h"
+#include "tree-cfg.h"
+#include "cfgloop.h"
+#include "tree-vectorizer.h"
+#include "tm_p.h"
+
+namespace {
+
+const pass_data pass_data_adjust_alignment =
+{
+  GIMPLE_PASS, /* type */
+  "adjust_alignment", /* name */
+  OPTGROUP_NONE, /* optinfo_flags */
+  TV_NONE, /* tv_id */
+  0, /* properties_required */
+  0, /* properties_provided */
+  0, /* properties_destroyed */
+  0, /* todo_flags_start */
+  0, /* todo_flags_finish */
+};
+
+class pass_adjust_alignment : public gimple_opt_pass
+{
+public:
+  pass_adjust_alignment (gcc::context *ctxt)
+    : gimple_opt_pass (pass_data_adjust_alignment, ctxt)
+  {}
+
+  virtual unsigned int execute (function *);
+
+}; // class pass_adjust_alignment
+
+} // anon namespace
+
+/* Entry point to adjust_alignment pass.  */
+unsigned int
+pass_adjust_alignment::execute (function *fun) {
+  size_t i;
+  tree var;
+  struct cgraph_node *node;
+  unsigned int align;
+
+  FOR_EACH_LOCAL_DECL (fun, i, var)
+    {
+      align = LOCAL_DECL_ALIGNMENT (var);
+
+      gcc_assert (align >= DECL_ALIGN (var));
+
+      SET_DECL_ALIGN (var, align);
+    }
+  return 0;
+}
+
+gimple_opt_pass *
+make_pass_adjust_alignment (gcc::context *ctxt)
+{
+  return new pass_adjust_alignment (ctxt);
+}
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index a1207a20a3c..576b3f67434 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -480,6 +480,7 @@ extern gimple_opt_pass *make_pass_sprintf_length (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_walloca (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_coroutine_lower_builtins (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_coroutine_early_expand_ifns (gcc::context *ctxt);
+extern gimple_opt_pass *make_pass_adjust_alignment (gcc::context *ctxt);
 
 /* IPA Passes */
 extern simple_ipa_opt_pass *make_pass_ipa_lower_emutls (gcc::context *ctxt);
-- 
2.25.2

Re: [PATCH v3] Fix alignment for local variable [PR90811]

[Kito Cheng]

ping

On Tue, Mar 31, 2020 at 4:33 PM Kito Cheng <kito.cheng@sifive.com> wrote:
>
>  - The alignment for local variable was adjust during estimate_stack_frame_size,
>    however it seems wrong spot to adjust that, expand phase will adjust that
>    but it little too late to some gimple optimization, which rely on certain
>    target hooks need to check alignment, forwprop is an example for
>    that, result of simplify_builtin_call rely on the alignment on some
>    target like ARM or RISC-V.
>
>  - This patch fix gfortran.dg/pr45636.f90 for arm and riscv.
>
>  - Regression test on riscv32/riscv64 and x86_64-linux-gnu, no new fail
>    introduced.
>
> gcc/ChangeLog
>
>         PR target/90811
>         * Makefile.in (OBJS): Add tree-adjust-alignment.o.
>         * tree-adjust-alignment.cc (pass_data_adjust_alignment): New.
>         (pass_adjust_alignment): New.
>         (-pass_adjust_alignment::execute): New.
>         (make_pass_adjust_alignment): New.
>         * tree-pass.h (make_pass_adjust_alignment): New.
>         * passes.def: Add pass_adjust_alignment.
> ---
>  gcc/Makefile.in              |  1 +
>  gcc/passes.def               |  1 +
>  gcc/tree-adjust-alignment.cc | 88 ++++++++++++++++++++++++++++++++++++
>  gcc/tree-pass.h              |  1 +
>  4 files changed, 91 insertions(+)
>  create mode 100644 gcc/tree-adjust-alignment.cc
>
> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> index fa9923bb270..9b73288f776 100644
> --- a/gcc/Makefile.in
> +++ b/gcc/Makefile.in
> @@ -1545,6 +1545,7 @@ OBJS = \
>         ubsan.o \
>         sanopt.o \
>         sancov.o \
> +       tree-adjust-alignment.o \
>         tree-call-cdce.o \
>         tree-cfg.o \
>         tree-cfgcleanup.o \
> diff --git a/gcc/passes.def b/gcc/passes.def
> index 2bf2cb78fc5..92cbe587a8a 100644
> --- a/gcc/passes.def
> +++ b/gcc/passes.def
> @@ -183,6 +183,7 @@ along with GCC; see the file COPYING3.  If not see
>    NEXT_PASS (pass_oacc_device_lower);
>    NEXT_PASS (pass_omp_device_lower);
>    NEXT_PASS (pass_omp_target_link);
> +  NEXT_PASS (pass_adjust_alignment);
>    NEXT_PASS (pass_all_optimizations);
>    PUSH_INSERT_PASSES_WITHIN (pass_all_optimizations)
>        NEXT_PASS (pass_remove_cgraph_callee_edges);
> diff --git a/gcc/tree-adjust-alignment.cc b/gcc/tree-adjust-alignment.cc
> new file mode 100644
> index 00000000000..1269f93e56a
> --- /dev/null
> +++ b/gcc/tree-adjust-alignment.cc
> @@ -0,0 +1,88 @@
> +/* Adjust alignment for local variable.
> +   Copyright (C) 2003-2020 Free Software Foundation, Inc.
> +   Contributed by Dorit Naishlos <dorit@il.ibm.com>
> +
> +This file is part of GCC.
> +
> +GCC is free software; you can redistribute it and/or modify it under
> +the terms of the GNU General Public License as published by the Free
> +Software Foundation; either version 3, or (at your option) any later
> +version.
> +
> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> +WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> +for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with GCC; see the file COPYING3.  If not see
> +<http://www.gnu.org/licenses/>.  */
> +
> +#include "config.h"
> +#include "system.h"
> +#include "coretypes.h"
> +#include "backend.h"
> +#include "target.h"
> +#include "tree.h"
> +#include "gimple.h"
> +#include "tree-pass.h"
> +#include "cgraph.h"
> +#include "fold-const.h"
> +#include "gimple-iterator.h"
> +#include "tree-cfg.h"
> +#include "cfgloop.h"
> +#include "tree-vectorizer.h"
> +#include "tm_p.h"
> +
> +namespace {
> +
> +const pass_data pass_data_adjust_alignment =
> +{
> +  GIMPLE_PASS, /* type */
> +  "adjust_alignment", /* name */
> +  OPTGROUP_NONE, /* optinfo_flags */
> +  TV_NONE, /* tv_id */
> +  0, /* properties_required */
> +  0, /* properties_provided */
> +  0, /* properties_destroyed */
> +  0, /* todo_flags_start */
> +  0, /* todo_flags_finish */
> +};
> +
> +class pass_adjust_alignment : public gimple_opt_pass
> +{
> +public:
> +  pass_adjust_alignment (gcc::context *ctxt)
> +    : gimple_opt_pass (pass_data_adjust_alignment, ctxt)
> +  {}
> +
> +  virtual unsigned int execute (function *);
> +
> +}; // class pass_adjust_alignment
> +
> +} // anon namespace
> +
> +/* Entry point to adjust_alignment pass.  */
> +unsigned int
> +pass_adjust_alignment::execute (function *fun) {
> +  size_t i;
> +  tree var;
> +  struct cgraph_node *node;
> +  unsigned int align;
> +
> +  FOR_EACH_LOCAL_DECL (fun, i, var)
> +    {
> +      align = LOCAL_DECL_ALIGNMENT (var);
> +
> +      gcc_assert (align >= DECL_ALIGN (var));
> +
> +      SET_DECL_ALIGN (var, align);
> +    }
> +  return 0;
> +}
> +
> +gimple_opt_pass *
> +make_pass_adjust_alignment (gcc::context *ctxt)
> +{
> +  return new pass_adjust_alignment (ctxt);
> +}
> diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
> index a1207a20a3c..576b3f67434 100644
> --- a/gcc/tree-pass.h
> +++ b/gcc/tree-pass.h
> @@ -480,6 +480,7 @@ extern gimple_opt_pass *make_pass_sprintf_length (gcc::context *ctxt);
>  extern gimple_opt_pass *make_pass_walloca (gcc::context *ctxt);
>  extern gimple_opt_pass *make_pass_coroutine_lower_builtins (gcc::context *ctxt);
>  extern gimple_opt_pass *make_pass_coroutine_early_expand_ifns (gcc::context *ctxt);
> +extern gimple_opt_pass *make_pass_adjust_alignment (gcc::context *ctxt);
>
>  /* IPA Passes */
>  extern simple_ipa_opt_pass *make_pass_ipa_lower_emutls (gcc::context *ctxt);
> --
> 2.25.2
>

Re: [PATCH v3] Fix alignment for local variable [PR90811]

[Richard Biener]

On Wed, Apr 8, 2020 at 7:13 PM Kito Cheng <kito.cheng@sifive.com> wrote:
>
> ping
>
> On Tue, Mar 31, 2020 at 4:33 PM Kito Cheng <kito.cheng@sifive.com> wrote:
> >
> >  - The alignment for local variable was adjust during estimate_stack_frame_size,
> >    however it seems wrong spot to adjust that, expand phase will adjust that
> >    but it little too late to some gimple optimization, which rely on certain
> >    target hooks need to check alignment, forwprop is an example for
> >    that, result of simplify_builtin_call rely on the alignment on some
> >    target like ARM or RISC-V.
> >
> >  - This patch fix gfortran.dg/pr45636.f90 for arm and riscv.
> >
> >  - Regression test on riscv32/riscv64 and x86_64-linux-gnu, no new fail
> >    introduced.
> >
> > gcc/ChangeLog
> >
> >         PR target/90811
> >         * Makefile.in (OBJS): Add tree-adjust-alignment.o.
> >         * tree-adjust-alignment.cc (pass_data_adjust_alignment): New.
> >         (pass_adjust_alignment): New.
> >         (-pass_adjust_alignment::execute): New.
> >         (make_pass_adjust_alignment): New.
> >         * tree-pass.h (make_pass_adjust_alignment): New.
> >         * passes.def: Add pass_adjust_alignment.
> > ---
> >  gcc/Makefile.in              |  1 +
> >  gcc/passes.def               |  1 +
> >  gcc/tree-adjust-alignment.cc | 88 ++++++++++++++++++++++++++++++++++++
> >  gcc/tree-pass.h              |  1 +
> >  4 files changed, 91 insertions(+)
> >  create mode 100644 gcc/tree-adjust-alignment.cc
> >
> > diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> > index fa9923bb270..9b73288f776 100644
> > --- a/gcc/Makefile.in
> > +++ b/gcc/Makefile.in
> > @@ -1545,6 +1545,7 @@ OBJS = \
> >         ubsan.o \
> >         sanopt.o \
> >         sancov.o \
> > +       tree-adjust-alignment.o \
> >         tree-call-cdce.o \
> >         tree-cfg.o \
> >         tree-cfgcleanup.o \
> > diff --git a/gcc/passes.def b/gcc/passes.def
> > index 2bf2cb78fc5..92cbe587a8a 100644
> > --- a/gcc/passes.def
> > +++ b/gcc/passes.def
> > @@ -183,6 +183,7 @@ along with GCC; see the file COPYING3.  If not see
> >    NEXT_PASS (pass_oacc_device_lower);
> >    NEXT_PASS (pass_omp_device_lower);
> >    NEXT_PASS (pass_omp_target_link);
> > +  NEXT_PASS (pass_adjust_alignment);
> >    NEXT_PASS (pass_all_optimizations);
> >    PUSH_INSERT_PASSES_WITHIN (pass_all_optimizations)
> >        NEXT_PASS (pass_remove_cgraph_callee_edges);
> > diff --git a/gcc/tree-adjust-alignment.cc b/gcc/tree-adjust-alignment.cc
> > new file mode 100644
> > index 00000000000..1269f93e56a
> > --- /dev/null
> > +++ b/gcc/tree-adjust-alignment.cc
> > @@ -0,0 +1,88 @@
> > +/* Adjust alignment for local variable.
> > +   Copyright (C) 2003-2020 Free Software Foundation, Inc.
> > +   Contributed by Dorit Naishlos <dorit@il.ibm.com>
> > +
> > +This file is part of GCC.
> > +
> > +GCC is free software; you can redistribute it and/or modify it under
> > +the terms of the GNU General Public License as published by the Free
> > +Software Foundation; either version 3, or (at your option) any later
> > +version.
> > +
> > +GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> > +WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > +FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> > +for more details.
> > +
> > +You should have received a copy of the GNU General Public License
> > +along with GCC; see the file COPYING3.  If not see
> > +<http://www.gnu.org/licenses/>.  */
> > +
> > +#include "config.h"
> > +#include "system.h"
> > +#include "coretypes.h"
> > +#include "backend.h"
> > +#include "target.h"
> > +#include "tree.h"
> > +#include "gimple.h"
> > +#include "tree-pass.h"
> > +#include "cgraph.h"
> > +#include "fold-const.h"
> > +#include "gimple-iterator.h"
> > +#include "tree-cfg.h"
> > +#include "cfgloop.h"
> > +#include "tree-vectorizer.h"
> > +#include "tm_p.h"
> > +
> > +namespace {
> > +
> > +const pass_data pass_data_adjust_alignment =
> > +{
> > +  GIMPLE_PASS, /* type */
> > +  "adjust_alignment", /* name */
> > +  OPTGROUP_NONE, /* optinfo_flags */
> > +  TV_NONE, /* tv_id */
> > +  0, /* properties_required */
> > +  0, /* properties_provided */
> > +  0, /* properties_destroyed */
> > +  0, /* todo_flags_start */
> > +  0, /* todo_flags_finish */
> > +};
> > +
> > +class pass_adjust_alignment : public gimple_opt_pass
> > +{
> > +public:
> > +  pass_adjust_alignment (gcc::context *ctxt)
> > +    : gimple_opt_pass (pass_data_adjust_alignment, ctxt)
> > +  {}
> > +
> > +  virtual unsigned int execute (function *);
> > +
> > +}; // class pass_adjust_alignment
> > +
> > +} // anon namespace
> > +
> > +/* Entry point to adjust_alignment pass.  */
> > +unsigned int
> > +pass_adjust_alignment::execute (function *fun) {
> > +  size_t i;
> > +  tree var;
> > +  struct cgraph_node *node;
> > +  unsigned int align;
> > +
> > +  FOR_EACH_LOCAL_DECL (fun, i, var)
> > +    {

Please skip vars with DECL_USER_ALIGN.

Note local-decls also contains the base variables that
are written into SSA form so you are processing variables
here that are not processed by the corresponding code
during RTL expansion.  It's probably harmless to increase
alignment of registers since that should be ignored.
I'm not sure whether alignment has any effect on
DECL_HARD_REGISTER vars.

Note local-decls also contains local statics.  I'm not sure
LOCAL_DECL_ALIGNMENT is supposed to handle those,
certainly the RTL expansion code does not.  So please
exclude is_global_var (var) vars as well.

Thanks,
Richard.

> > +      align = LOCAL_DECL_ALIGNMENT (var);
> > +
> > +      gcc_assert (align >= DECL_ALIGN (var));
> > +
> > +      SET_DECL_ALIGN (var, align);
> > +    }
> > +  return 0;
> > +}
> > +
> > +gimple_opt_pass *
> > +make_pass_adjust_alignment (gcc::context *ctxt)
> > +{
> > +  return new pass_adjust_alignment (ctxt);
> > +}
> > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
> > index a1207a20a3c..576b3f67434 100644
> > --- a/gcc/tree-pass.h
> > +++ b/gcc/tree-pass.h
> > @@ -480,6 +480,7 @@ extern gimple_opt_pass *make_pass_sprintf_length (gcc::context *ctxt);
> >  extern gimple_opt_pass *make_pass_walloca (gcc::context *ctxt);
> >  extern gimple_opt_pass *make_pass_coroutine_lower_builtins (gcc::context *ctxt);
> >  extern gimple_opt_pass *make_pass_coroutine_early_expand_ifns (gcc::context *ctxt);
> > +extern gimple_opt_pass *make_pass_adjust_alignment (gcc::context *ctxt);
> >
> >  /* IPA Passes */
> >  extern simple_ipa_opt_pass *make_pass_ipa_lower_emutls (gcc::context *ctxt);
> > --
> > 2.25.2
> >

Re: [PATCH v3] Fix alignment for local variable [PR90811]

[Kito Cheng]

Hi Richard:

> Please skip vars with DECL_USER_ALIGN.

I was consider to skip DECL_USER_ALIGN,
but Jakub think it shouldn't skip in other patch review[1] (related to
this patch),
because it's minimum alignment not restricted alignment[2]

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

>
> Note local-decls also contains the base variables that
> are written into SSA form so you are processing variables
> here that are not processed by the corresponding code
> during RTL expansion.  It's probably harmless to increase
> alignment of registers since that should be ignored.
> I'm not sure whether alignment has any effect on
> DECL_HARD_REGISTER vars.

I did more checks on expand_one_var, DECL_HARD_REGISTER won't adjust
alignment since use different path to expand, so it should skip here.

So yeah, it seems should skip here.

> Note local-decls also contains local statics.  I'm not sure
> LOCAL_DECL_ALIGNMENT is supposed to handle those,
> certainly the RTL expansion code does not.  So please
> exclude is_global_var (var) vars as well.

I checked static local will included in FOR_EACH_LOCAL_DECL, will skip
is_global_var next version

Thanks :)