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

[Richard Biener via Gcc-patches]

On Wed, May 20, 2020 at 2:30 PM Kito Cheng  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  wrote:
> >
> > Ok, I will check.
> >
> > On Wed, May 20, 2020 at 8:04 PM Richard Biener
> >  wrote:
> > >
> > > On Wed, May 20, 2020 at 9:20 AM Kito Cheng  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
> > > >  wrote:
> > > > >
> > > > > On Mon, May 18, 2020 at 9:27 AM Kito Cheng  
> > > > > wrote:
> > > > > >
> > > > > > ping
> > > > > >
> > > > > > On Tue, Apr 14, 2020 at 2:53 PM Kito Cheng  
> > > > > > 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 

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

[Kito Cheng]

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.


On Wed, May 20, 2020 at 8:06 PM Kito Cheng  wrote:
>
> Ok, I will check.
>
> On Wed, May 20, 2020 at 8:04 PM Richard Biener
>  wrote:
> >
> > On Wed, May 20, 2020 at 9:20 AM Kito Cheng  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
> > >  wrote:
> > > >
> > > > On Mon, May 18, 2020 at 9:27 AM Kito Cheng  
> > > > wrote:
> > > > >
> > > > > ping
> > > > >
> > > > > On Tue, Apr 14, 2020 at 2:53 PM Kito Cheng  
> > > > > 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 000..77f38f6949b
> > > > >> --- /dev/null
> > > > >> +++ b/gcc/tree-adjust-alignment.cc
> > > > >> @@ -0,0 +1,92 @@
> > > > >> +/* Adjust 

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

[Kito Cheng]

Ok, I will check.

On Wed, May 20, 2020 at 8:04 PM Richard Biener
 wrote:
>
> On Wed, May 20, 2020 at 9:20 AM Kito Cheng  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
> >  wrote:
> > >
> > > On Mon, May 18, 2020 at 9:27 AM Kito Cheng  wrote:
> > > >
> > > > ping
> > > >
> > > > On Tue, Apr 14, 2020 at 2:53 PM Kito Cheng  
> > > > 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 000..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 
> > >
> > > 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 

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

[Richard Biener via Gcc-patches]

On Wed, May 20, 2020 at 9:20 AM Kito Cheng  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
>  wrote:
> >
> > On Mon, May 18, 2020 at 9:27 AM Kito Cheng  wrote:
> > >
> > > ping
> > >
> > > On Tue, Apr 14, 2020 at 2:53 PM Kito Cheng  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 000..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 
> >
> > 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
> > 

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

[Kito Cheng]

Hi Richard:

Tested and committed with fixes, thanks your review :)

On Mon, May 18, 2020 at 6:22 PM Richard Biener
 wrote:
>
> On Mon, May 18, 2020 at 9:27 AM Kito Cheng  wrote:
> >
> > ping
> >
> > On Tue, Apr 14, 2020 at 2:53 PM Kito Cheng  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 000..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 
>
> 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
> >> +.  */
> >> +
> >> +#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)
> 

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

[Richard Biener via Gcc-patches]

On Mon, May 18, 2020 at 9:27 AM Kito Cheng  wrote:
>
> ping
>
> On Tue, Apr 14, 2020 at 2:53 PM Kito Cheng  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 000..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 

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
>> +.  */
>> +
>> +#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

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

[Kito Cheng]

ping

On Tue, Apr 14, 2020 at 2:53 PM Kito Cheng  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 \
> 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 000..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.
> +   Contributed by Dorit Naishlos 
> +
> +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
> +.  */
> +
> +#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;
> +  unsigned int align;
> +
> +  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);
> +
> +  /* Make sure alignment only increase.  */
> +  gcc_assert (align >= DECL_ALIGN (var));
> +
> +  SET_DECL_ALIGN (var, align);
> +}
> 

[PATCH v4] 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.

 - 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 \
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 000..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.
+   Contributed by Dorit Naishlos 
+
+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
+.  */
+
+#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;
+  unsigned int align;
+
+  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);
+
+  /* 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);