On Wed, May 20, 2020 at 2:30 PM Kito Cheng <kito.ch...@sifive.com> wrote: > > There is an assertion checking to make sure LOCAL_DECL_ALIGNMENT never > shrink alignment, > > But those two testcase did under x86_64 with -m32, I am not sure if > the behavior is expected or not? > It should be fixed on target if that behavior is not expected.
Well, it happens that it makes what I said upon discussing the patch true - we should set alignment during decl layout. Here the compile is with -mpreferred-stack-boundary=2 so we cannot get 8 byte alignment unless we align the stack dynamically which is undesirable. But on 32bit long long is naturally aligned. I guess this was a latent issue before, since we didn't have the assert. I'll open a bug. Richard. > > On Wed, May 20, 2020 at 8:06 PM Kito Cheng <kito.ch...@sifive.com> wrote: > > > > Ok, I will check. > > > > On Wed, May 20, 2020 at 8:04 PM Richard Biener > > <richard.guent...@gmail.com> wrote: > > > > > > On Wed, May 20, 2020 at 9:20 AM Kito Cheng <kito.ch...@sifive.com> wrote: > > > > > > > > Hi Richard: > > > > > > > > Tested and committed with fixes, thanks your review :) > > > > > > And we're now hitting > > > > > > internal compiler error: in execute, at adjust-alignment.c:73 > > > > > > for > > > > > > FAIL: gcc.target/i386/pr69454-2.c (internal compiler error) > > > FAIL: gcc.target/i386/pr69454-2.c (test for excess errors) > > > UNRESOLVED: gcc.target/i386/stackalign/longlong-1.c -mno-stackrealign > > > scan-assembler-not and[lq]?[^\\\\n]*-8,[^\\\\n]*sp > > > FAIL: gcc.target/i386/stackalign/longlong-1.c -mno-stackrealign > > > (internal compiler error) > > > FAIL: gcc.target/i386/stackalign/longlong-1.c -mno-stackrealign (test > > > for excess errors) > > > UNRESOLVED: gcc.target/i386/stackalign/longlong-1.c -mstackrealign > > > scan-assembler-not and[lq]?[^\\\\n]*-8,[^\\\\n]*sp > > > FAIL: gcc.target/i386/stackalign/longlong-1.c -mstackrealign (internal > > > compiler error) > > > FAIL: gcc.target/i386/stackalign/longlong-1.c -mstackrealign (test for > > > excess errors) > > > > > > on x86_64 with -m32 > > > > > > Can you please investigate? > > > > > > Thanks, > > > Richard. > > > > > > > On Mon, May 18, 2020 at 6:22 PM Richard Biener > > > > <richard.guent...@gmail.com> wrote: > > > > > > > > > > On Mon, May 18, 2020 at 9:27 AM Kito Cheng <kito.ch...@sifive.com> > > > > > wrote: > > > > > > > > > > > > ping > > > > > > > > > > > > On Tue, Apr 14, 2020 at 2:53 PM Kito Cheng <kito.ch...@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. > > > > > >> > > > > > >> - Exclude static local var and hard register var in the process of > > > > > >> alignment adjustment. > > > > > >> > > > > > >> - 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 | 92 > > > > > >> ++++++++++++++++++++++++++++++++++++ > > > > > >> gcc/tree-pass.h | 1 + > > > > > >> 4 files changed, 95 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 \ > > > > > > > > > > Please rename the file to just 'adjust-alignment.c' > > > > > > > > > > >> 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..77f38f6949b > > > > > >> --- /dev/null > > > > > >> +++ b/gcc/tree-adjust-alignment.cc > > > > > >> @@ -0,0 +1,92 @@ > > > > > >> +/* Adjust alignment for local variable. > > > > > >> + Copyright (C) 2003-2020 Free Software Foundation, Inc. > > > > > > > > > > Just 2020 please > > > > > > > > > > >> + Contributed by Dorit Naishlos <do...@il.ibm.com> > > > > > > > > > > Eh? Please put in your name. > > > > > > > > > > >> + > > > > > >> +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" > > > > > > > > > > Did you try to narrow down the set of includes? > > > > > Please do so, tree-vectorizer.h does not look needed. > > > > > > > > > > >> +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 *); > > > > > >> + > > > > > > > > > > excessive vertical space > > > > > > > > > > >> +}; // class pass_adjust_alignment > > > > > >> + > > > > > >> +} // anon namespace > > > > > >> + > > > > > >> +/* Entry point to adjust_alignment pass. */ > > > > > >> +unsigned int > > > > > >> +pass_adjust_alignment::execute (function *fun) { > > > > > > > > > > The { goes to the next line and the following lines indents > > > > > are off then. > > > > > > > > > > >> + size_t i; > > > > > >> + tree var; > > > > > >> + unsigned int align; > > > > > > > > > > declare variables where they are used, thus... > > > > > > > > > > >> + > > > > > >> + FOR_EACH_LOCAL_DECL (fun, i, var) > > > > > >> + { > > > > > >> + /* Don't adjust aligment for static local var and hard > > > > > >> register var. */ > > > > > >> + if (is_global_var (var) || DECL_HARD_REGISTER (var)) > > > > > >> + continue; > > > > > >> + > > > > > >> + align = LOCAL_DECL_ALIGNMENT (var); > > > > > > > > > > unsigned align = LOCAL... > > > > > > > > > > for example. > > > > > > > > > > OK with those changes. > > > > > > > > > > Thanks, > > > > > Richard. > > > > > > > > > > >> + > > > > > >> + /* Make sure alignment only increase. */ > > > > > >> + 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.26.0 > > > > > >>
