Re: [r11-3641 Regression] FAIL: gcc.dg/torture/pta-ptrarith-1.c -Os scan-tree-dump alias "ESCAPED = {[^\n}]* i f [^\n}]*}" on Linux/x86_64 (-m32 -march=cascadelake)
On Sun, Oct 4, 2020 at 10:41 AM H.J. Lu via Gcc-patches < gcc-patches@gcc.gnu.org> wrote: > On Sun, Oct 4, 2020 at 10:33 AM Martin Sebor wrote: > > > > On 10/4/20 10:51 AM, H.J. Lu via Gcc-patches wrote: > > > On Sat, Oct 3, 2020 at 5:57 PM Segher Boessenkool > > > wrote: > > >> > > >> On Sat, Oct 03, 2020 at 12:21:04PM -0700, sunil.k.pandey via > Gcc-patches wrote: > > >>> On Linux/x86_64, > > >>> > > >>> c34db4b6f8a5d80367c709309f9b00cb32630054 is the first bad commit > > >>> commit c34db4b6f8a5d80367c709309f9b00cb32630054 > > >>> Author: Jan Hubicka > > >>> Date: Sat Oct 3 17:20:16 2020 +0200 > > >>> > > >>> Track access ranges in ipa-modref > > >>> > > >>> caused > > >> > > >> [ ... ] > > >> > > >> This isn't a patch. Wrong mailing list? > > > > > > I view this as a follow up of > > > > > > https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555314.html > > > > > > What do people think about this kind of followups? Is this appropriate > > > for this mailing list? > > > > A number of people routinely send emails similar to these to this > > list to point out regressions on their targets. I find both kinds > > of emails very useful and don't mind the additional traffic. > > > > What would be an improvement is sending just one email for all > > the testsuite regressions rather than one for each test or run > > as seems to be happening. > > Sunil, can you update your script to send out a single email > for all regressions caused by one commit? Thanks. > Sure, I'll work on it. > > > I'm not sure that automatically opening bugs instead would be > > better, certainly not one per test, and not if the code author > > wasn't also CC'd if not automatically assigned to it. > > > > Martin > > > > -- > H.J. >
Re: [r12-989 Regression] FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable -Os (test for warnings, line 98) on Linux/x86_64
Hi Thomas, I reproduced this issue manually and it turns out this is a special case. Script takes input from https://gcc.gnu.org/pipermail/gcc-regression/ and it matches the exact error message in the triaging process. This failure reported on gcc regression https://gcc.gnu.org/pipermail/gcc-regression/2021-May/074806.html Reason it triaged 325aa13996bafce0c4927876c315d1fa706d9881 and not 11b8286a83289f5b54e813f14ff56d730c3f3185 because, Commit 325aa13996bafce0c4927876c315d1fa706d9881 is the first commit which matches the failure message reported on gcc-regression. See the difference in line number. Error message produced from commit 325aa13996bafce0c4927876c315d1fa706d9881: FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable -Os (test for warnings, line 98) FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable -Os (test for warnings, line 134) vs. Error message produced from commit 11b8286a83289f5b54e813f14ff56d730c3f3185: FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable -Os (test for warnings, line 100) FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable -Os (test for warnings, line 136) Thank you so much, Sunil Pandey On Sat, May 22, 2021 at 1:41 AM Thomas Schwinge wrote: > Hi! > > First: many thanks for running this automated regression testing > machinery! > > On 2021-05-21T18:40:55-0700, "sunil.k.pandey via Gcc-patches" < > gcc-patches@gcc.gnu.org> wrote: > > On Linux/x86_64, > > > > 325aa13996bafce0c4927876c315d1fa706d9881 is the first bad commit > > commit 325aa13996bafce0c4927876c315d1fa706d9881 > > Author: Thomas Schwinge > > Date: Fri May 21 08:51:47 2021 +0200 > > > > [OpenACC privatization] Reject 'static', 'external' in blocks > [PR90115] > > Actually not that one, but instead one commit before is the culprit: > > commit 11b8286a83289f5b54e813f14ff56d730c3f3185 > Author: Thomas Schwinge > Date: Thu May 20 16:11:37 2021 +0200 > > [OpenACC privatization] Largely extend diagnostics and > corresponding testsuite coverage [PR90115] > > (Probably your testing aggregates commits that appear in some period of > time? Maybe reflect that in the reporting emails?) > > > caused > > > > FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 > -DACC_MEM_SHARED=1 -foffload=disable -O0 (test for warnings, line 134) > > FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 > -DACC_MEM_SHARED=1 -foffload=disable -O0 (test for warnings, line 98) > > FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 > -DACC_MEM_SHARED=1 -foffload=disable -O1 (test for warnings, line 134) > > FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 > -DACC_MEM_SHARED=1 -foffload=disable -O1 (test for warnings, line 98) > > FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 > -DACC_MEM_SHARED=1 -foffload=disable -O2 (test for warnings, line 134) > > FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 > -DACC_MEM_SHARED=1 -foffload=disable -O2 (test for warnings, line 98) > > FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 > -DACC_MEM_SHARED=1 -foffload=disable -O3 -fomit-frame-pointer > -funroll-loops -fpeel-loops -ftracer -finline-functions (test for > warnings, line 134) > > FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 > -DACC_MEM_SHARED=1 -foffload=disable -O3 -fomit-frame-pointer > -funroll-loops -fpeel-loops -ftracer -finline-functions (test for > warnings, line 98) > > FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 > -DACC_MEM_SHARED=1 -foffload=disable -O3 -g (test for warnings, line 134) > > FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 > -DACC_MEM_SHARED=1 -foffload=disable -O3 -g (test for warnings, line 98) > > FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 > -DACC_MEM_SHARED=1 -foffload=disable -Os (test for warnings, line 134) > > FAIL: libgomp.oacc-fortran/privatized-ref-2.f90 -DACC_DEVICE_TYPE_host=1 > -DACC_MEM_SHARED=1 -foffload=disable -Os (test for warnings, line 98) > > Sorry, and ACK, and I'm confused why I didn't see that in my own testing. > I've now pushed "[OpenACC privatization] Prune uninteresting/varying > diagnostics in 'libgomp.oacc-fortran/privatized-ref-2.f90'" to master > branch in commit 3050a1a18276d7cdd8946e34cc1344e30efb7030, see attached. > > > Grüße > Thomas > > > - > Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München > Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank > Thürauf >
Re: [PATCH] Preliminary work on support for 128bits integers
Sorry, I made a mistake. Please ignore it. On Fri, Sep 11, 2020 at 9:06 AM Sunil K Pandey via Gcc-patches < gcc-patches@gcc.gnu.org> wrote: > From: Arnaud Charlet > > * fe.h, opt.ads (Enable_128bit_Types): New. > * stand.ads (Standard_Long_Long_Long_Integer, > S_Long_Long_Long_Integer): New. > --- > gcc/ada/fe.h | 1 + > gcc/ada/opt.ads | 7 +++ > gcc/ada/stand.ads | 4 > 3 files changed, 12 insertions(+) > > diff --git a/gcc/ada/fe.h b/gcc/ada/fe.h > index 8ad16c2b1c9..520301e4c3e 100644 > --- a/gcc/ada/fe.h > +++ b/gcc/ada/fe.h > @@ -192,6 +192,7 @@ extern Boolean In_Extended_Main_Code_Unit > (Entity_Id); > #define Ada_Versionopt__ada_version > #define Back_End_Inlining opt__back_end_inlining > #define Debug_Generated_Code opt__debug_generated_code > +#define Enable_128bit_Typesopt__enable_128bit_types > #define Exception_Extra_Info opt__exception_extra_info > #define Exception_Locations_Suppressed opt__exception_locations_suppressed > #define Exception_Mechanismopt__exception_mechanism > diff --git a/gcc/ada/opt.ads b/gcc/ada/opt.ads > index c982f83b9e4..885a6fb9497 100644 > --- a/gcc/ada/opt.ads > +++ b/gcc/ada/opt.ads > @@ -525,6 +525,13 @@ package Opt is > -- dataflow analysis, which is not available. This behavior parallels > that > -- of the old ABE mechanism. > > + Enable_128bit_Types : Boolean := False; > + -- GNAT > + -- Set to True to enable the support for 128-bit types in the > compiler. > + -- The prerequisite is a 64-bit target that supports 128-bit > computation. > + > + -- WARNING: There is a matching C declaration of this variable in fe.h > + > Error_Msg_Line_Length : Nat := 0; > -- GNAT > -- Records the error message line length limit. If this is set to > zero, > diff --git a/gcc/ada/stand.ads b/gcc/ada/stand.ads > index f3f7eb512d5..57b4d55387e 100644 > --- a/gcc/ada/stand.ads > +++ b/gcc/ada/stand.ads > @@ -61,6 +61,7 @@ package Stand is >S_Integer, >S_Long_Integer, >S_Long_Long_Integer, > + S_Long_Long_Long_Integer, > >S_Natural, >S_Positive, > @@ -283,6 +284,9 @@ package Stand is > Standard_Long_Integer: Entity_Id renames SE (S_Long_Integer); > Standard_Long_Long_Integer : Entity_Id renames SE > (S_Long_Long_Integer); > > + Standard_Long_Long_Long_Integer : Entity_Id renames > + SE > (S_Long_Long_Long_Integer); > + > Standard_Op_Add : Entity_Id renames SE (S_Op_Add); > Standard_Op_And : Entity_Id renames SE (S_Op_And); > Standard_Op_Concat : Entity_Id renames SE (S_Op_Concat); > -- > 2.26.2 > >
Re: [PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237]
On Sat, Jul 4, 2020 at 9:11 AM Richard Biener wrote: > > On July 3, 2020 11:16:46 PM GMT+02:00, Jason Merrill wrote: > >On 6/29/20 5:00 AM, Richard Biener wrote: > >> On Fri, Jun 26, 2020 at 10:11 PM H.J. Lu wrote: > >>> > >>> On Thu, Jun 25, 2020 at 1:10 AM Richard Biener > >>> wrote: > > On Thu, Jun 25, 2020 at 2:53 AM Sunil Pandey > >wrote: > > > > On Wed, Jun 24, 2020 at 12:30 AM Richard Biener > > wrote: > >> > >> On Tue, Jun 23, 2020 at 5:31 PM Sunil K Pandey via Gcc-patches > >> wrote: > >>> > >>> From: Sunil K Pandey > >>> > >>> Default for this hook is NOP. For x86, in 32 bit mode, this hook > >>> sets alignment of long long on stack to 32 bits if preferred > >stack > >>> boundary is 32 bits. > >>> > >>> - This patch fixes > >>> gcc.target/i386/pr69454-2.c > >>> gcc.target/i386/stackalign/longlong-1.c > >>> - Regression test on x86-64, no new fail introduced. > >> > >> I think the name is badly chosen, > >TARGET_LOWER_LOCAL_DECL_ALIGNMENT > > > > Yes, I can change the target hook name. > > > >> would be better suited (and then asks for LOCAL_DECL_ALIGNMENT to > >be > >> renamed to INCREASE_LOCAL_DECL_ALIGNMENT). > > > > It seems like LOCAL_DECL_ALIGNMENT macro documentation is > >incorrect. > > It increases as well as decreases alignment based on > >condition(-m32 > > -mpreferred-stack-boundary=2) > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95885 > > > >> > >> You're calling it from do_type_align which IMHO is dangerous > >since that's > >> invoked from FIELD_DECL layout as well. Instead invoke it from > >> layout_decl itself where we do > >> > >>if (code != FIELD_DECL) > >> /* For non-fields, update the alignment from the type. */ > >> do_type_align (type, decl); > >> > >> and invoke the hook _after_ do_type_align. Also avoid > >> invoking the hook on globals or hard regs and only > >> invoke it on VAR_DECLs, thus only > >> > >>if (VAR_P (decl) && !is_global_var (decl) && > >!DECL_HARD_REGISTER (decl)) > > > > It seems like decl property is not fully populated at this point > >call > > to is_global_var (decl) on global variable return false. > > > > $ cat foo.c > > long long x; > > int main() > > { > > if (__alignof__(x) != 8) > >__builtin_abort(); > > return 0; > > } > > > > Breakpoint 1, layout_decl (decl=0x77ffbb40, known_align=0) > > at /local/skpandey/gccwork/gccwork/gcc/gcc/stor-layout.c:674 > > 674 do_type_align (type, decl); > > Missing separate debuginfos, use: dnf debuginfo-install > > gmp-6.1.2-10.fc31.x86_64 isl-0.16.1-9.fc31.x86_64 > > libmpc-1.1.0-4.fc31.x86_64 mpfr-3.1.6-5.fc31.x86_64 > > zlib-1.2.11-20.fc31.x86_64 > > (gdb) call debug_tree(decl) > >> type > size > > unit-size > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 > > canonical-type 0x7fffea801888 precision:64 min > 0x7fffea7e8fd8 -9223372036854775808> max >0x7fffea806000 > > 9223372036854775807> > > pointer_to_this > > > DI foo.c:1:11 size unit-size > > > > align:1 warn_if_not_align:0> > > > > (gdb) p is_global_var(decl) > > $1 = false > > (gdb) > > > > > > What about calling hook here > > > > 603 do_type_align (tree type, tree decl) > > 604 { > > 605 if (TYPE_ALIGN (type) > DECL_ALIGN (decl)) > > 606 { > > 607 SET_DECL_ALIGN (decl, TYPE_ALIGN (type)); > > 608 if (TREE_CODE (decl) == FIELD_DECL) > > 609 DECL_USER_ALIGN (decl) = TYPE_USER_ALIGN (type); > > 610 else > > 611 /* Lower local decl alignment */ > > 612 if (VAR_P (decl) > > 613 && !is_global_var (decl) > > 614 && !DECL_HARD_REGISTER (decl) > > 615 && cfun != NULL) > > 616 targetm.lower_local_decl_alignment (decl); > > 617 } > > But that doesn't change anything (obviously). layout_decl > is called quite early, too early it looks like. > > Now there doesn't seem to be any other good place where > we are sure to catch the decl before we evaluate things > like __alignof__ > > void __attribute__((noipa)) > foo (__SIZE_TYPE__ align, long long *p) > { > if ((__SIZE_TYPE__)p & (align-1)) > __builtin_abort (); > } > int main() > { > long long y; > foo (_Alignof y, &y); > return 0; > } > > Joseph/Jason - do you have a good recommendation > how to deal with targets where natural alignment > is supposed to be lowered for optimization purposes? > (this case is for i?8
Re: [PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237]
Any comment on revised patch? At least, in finish_decl, decl global attributes are populated. On Tue, Jul 14, 2020 at 8:37 AM Sunil Pandey wrote: > On Sat, Jul 4, 2020 at 9:11 AM Richard Biener > wrote: > > > > On July 3, 2020 11:16:46 PM GMT+02:00, Jason Merrill > wrote: > > >On 6/29/20 5:00 AM, Richard Biener wrote: > > >> On Fri, Jun 26, 2020 at 10:11 PM H.J. Lu wrote: > > >>> > > >>> On Thu, Jun 25, 2020 at 1:10 AM Richard Biener > > >>> wrote: > > > > On Thu, Jun 25, 2020 at 2:53 AM Sunil Pandey > > >wrote: > > > > > > On Wed, Jun 24, 2020 at 12:30 AM Richard Biener > > > wrote: > > >> > > >> On Tue, Jun 23, 2020 at 5:31 PM Sunil K Pandey via Gcc-patches > > >> wrote: > > >>> > > >>> From: Sunil K Pandey > > >>> > > >>> Default for this hook is NOP. For x86, in 32 bit mode, this hook > > >>> sets alignment of long long on stack to 32 bits if preferred > > >stack > > >>> boundary is 32 bits. > > >>> > > >>> - This patch fixes > > >>> gcc.target/i386/pr69454-2.c > > >>> gcc.target/i386/stackalign/longlong-1.c > > >>> - Regression test on x86-64, no new fail introduced. > > >> > > >> I think the name is badly chosen, > > >TARGET_LOWER_LOCAL_DECL_ALIGNMENT > > > > > > Yes, I can change the target hook name. > > > > > >> would be better suited (and then asks for LOCAL_DECL_ALIGNMENT to > > >be > > >> renamed to INCREASE_LOCAL_DECL_ALIGNMENT). > > > > > > It seems like LOCAL_DECL_ALIGNMENT macro documentation is > > >incorrect. > > > It increases as well as decreases alignment based on > > >condition(-m32 > > > -mpreferred-stack-boundary=2) > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95885 > > > > > >> > > >> You're calling it from do_type_align which IMHO is dangerous > > >since that's > > >> invoked from FIELD_DECL layout as well. Instead invoke it from > > >> layout_decl itself where we do > > >> > > >>if (code != FIELD_DECL) > > >> /* For non-fields, update the alignment from the type. */ > > >> do_type_align (type, decl); > > >> > > >> and invoke the hook _after_ do_type_align. Also avoid > > >> invoking the hook on globals or hard regs and only > > >> invoke it on VAR_DECLs, thus only > > >> > > >>if (VAR_P (decl) && !is_global_var (decl) && > > >!DECL_HARD_REGISTER (decl)) > > > > > > It seems like decl property is not fully populated at this point > > >call > > > to is_global_var (decl) on global variable return false. > > > > > > $ cat foo.c > > > long long x; > > > int main() > > > { > > > if (__alignof__(x) != 8) > > >__builtin_abort(); > > > return 0; > > > } > > > > > > Breakpoint 1, layout_decl (decl=0x77ffbb40, known_align=0) > > > at /local/skpandey/gccwork/gccwork/gcc/gcc/stor-layout.c:674 > > > 674 do_type_align (type, decl); > > > Missing separate debuginfos, use: dnf debuginfo-install > > > gmp-6.1.2-10.fc31.x86_64 isl-0.16.1-9.fc31.x86_64 > > > libmpc-1.1.0-4.fc31.x86_64 mpfr-3.1.6-5.fc31.x86_64 > > > zlib-1.2.11-20.fc31.x86_64 > > > (gdb) call debug_tree(decl) > > >> > type > > size > > > unit-size > > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 > > > canonical-type 0x7fffea801888 precision:64 min > > 0x7fffea7e8fd8 -9223372036854775808> max > >0x7fffea806000 > > > 9223372036854775807> > > > pointer_to_this > > > > DI foo.c:1:11 size unit-size > > > > > > align:1 warn_if_not_align:0> > > > > > > (gdb) p is_global_var(decl) > > > $1 = false > > > (gdb) > > > > > > > > > What about calling hook here > > > > > > 603 do_type_align (tree type, tree decl) > > > 604 { > > > 605 if (TYPE_ALIGN (type) > DECL_ALIGN (decl)) > > > 606 { > > > 607 SET_DECL_ALIGN (decl, TYPE_ALIGN (type)); > > > 608 if (TREE_CODE (decl) == FIELD_DECL) > > > 609 DECL_USER_ALIGN (decl) = TYPE_USER_ALIGN (type); > > > 610 else > > > 611 /* Lower local decl alignment */ > > > 612 if (VAR_P (decl) > > > 613 && !is_global_var (decl) > > > 614 && !DECL_HARD_REGISTER (decl) > > > 615 && cfun != NULL) > > > 616 targetm.lower_local_decl_alignment (decl); > > > 617 } > > > > But that doesn't change anything (obviously). layout_decl > > is called quite early, too early it looks like. > > > > Now there doesn't seem to be any other good place where > > we are sure to catch the decl before we evaluate things > > like __alignof__ > > > > void __attribute__((noipa)) > > foo (__SIZE_TYPE__ align, long long *p
Re: [PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237]
On Fri, Jul 17, 2020 at 1:22 AM Richard Biener wrote: > > On Fri, Jul 17, 2020 at 7:15 AM Sunil Pandey wrote: > > > > Any comment on revised patch? At least, in finish_decl, decl global > > attributes are populated. > > +static void > +ix86_lower_local_decl_alignment (tree decl) > +{ > + unsigned new_align = LOCAL_DECL_ALIGNMENT (decl); > > please use the macro-expanded call here since we want to amend > ix86_local_alignment to _not_ return a lower alignment when > called as LOCAL_DECL_ALIGNMENT (by adding a new parameter > to ix86_local_alignment). Can you also amend the patch in this > way? > > + if (new_align < DECL_ALIGN (decl)) > +SET_DECL_ALIGN (decl, new_align); > > diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c > index 81bd2ee94f0..1ae99e30ed1 100644 > --- a/gcc/c/c-decl.c > +++ b/gcc/c/c-decl.c > @@ -5601,6 +5601,8 @@ finish_decl (tree decl, location_t init_loc, tree init, > } > >invoke_plugin_callbacks (PLUGIN_FINISH_DECL, decl); > + /* Lower local decl alignment. */ > + lower_decl_alignment (decl); > } > > should come before plugin hook invocation, likewise for the cp_finish_decl > case. > > +/* Lower DECL alignment. */ > + > +void > +lower_decl_alignment (tree decl) > +{ > + if (VAR_P (decl) > + && !is_global_var (decl) > + && !DECL_HARD_REGISTER (decl)) > +targetm.lower_local_decl_alignment (decl); > +} > > please avoid this function, it's name sounds too generic and it's not worth > adding a public API for two calls. > > Alltogether this should avoid the x86 issue leaving left-overs (your > identified > inliner case) as missed optimization [for the linux kernel which appearantly > decided that -mpreferred-stack-boundary=2 is a good ABI to use]. > > Richard. > > Revised patch attached. > > On Tue, Jul 14, 2020 at 8:37 AM Sunil Pandey wrote: > >> > >> On Sat, Jul 4, 2020 at 9:11 AM Richard Biener > >> wrote: > >> > > >> > On July 3, 2020 11:16:46 PM GMT+02:00, Jason Merrill > >> > wrote: > >> > >On 6/29/20 5:00 AM, Richard Biener wrote: > >> > >> On Fri, Jun 26, 2020 at 10:11 PM H.J. Lu wrote: > >> > >>> > >> > >>> On Thu, Jun 25, 2020 at 1:10 AM Richard Biener > >> > >>> wrote: > >> > > >> > On Thu, Jun 25, 2020 at 2:53 AM Sunil Pandey > >> > >wrote: > >> > > > >> > > On Wed, Jun 24, 2020 at 12:30 AM Richard Biener > >> > > wrote: > >> > >> > >> > >> On Tue, Jun 23, 2020 at 5:31 PM Sunil K Pandey via Gcc-patches > >> > >> wrote: > >> > >>> > >> > >>> From: Sunil K Pandey > >> > >>> > >> > >>> Default for this hook is NOP. For x86, in 32 bit mode, this hook > >> > >>> sets alignment of long long on stack to 32 bits if preferred > >> > >stack > >> > >>> boundary is 32 bits. > >> > >>> > >> > >>> - This patch fixes > >> > >>> gcc.target/i386/pr69454-2.c > >> > >>> gcc.target/i386/stackalign/longlong-1.c > >> > >>> - Regression test on x86-64, no new fail introduced. > >> > >> > >> > >> I think the name is badly chosen, > >> > >TARGET_LOWER_LOCAL_DECL_ALIGNMENT > >> > > > >> > > Yes, I can change the target hook name. > >> > > > >> > >> would be better suited (and then asks for LOCAL_DECL_ALIGNMENT to > >> > >be > >> > >> renamed to INCREASE_LOCAL_DECL_ALIGNMENT). > >> > > > >> > > It seems like LOCAL_DECL_ALIGNMENT macro documentation is > >> > >incorrect. > >> > > It increases as well as decreases alignment based on > >> > >condition(-m32 > >> > > -mpreferred-stack-boundary=2) > >> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95885 > >> > > > >> > >> > >> > >> You're calling it from do_type_align which IMHO is dangerous > >> > >since that's > >> > >> invoked from FIELD_DECL layout as well. Instead invoke it from > >> > >> layout_decl itself where we do > >> > >> > >> > >>if (code != FIELD_DECL) > >> > >> /* For non-fields, update the alignment from the type. */ > >> > >> do_type_align (type, decl); > >> > >> > >> > >> and invoke the hook _after_ do_type_align. Also avoid > >> > >> invoking the hook on globals or hard regs and only > >> > >> invoke it on VAR_DECLs, thus only > >> > >> > >> > >>if (VAR_P (decl) && !is_global_var (decl) && > >> > >!DECL_HARD_REGISTER (decl)) > >> > > > >> > > It seems like decl property is not fully populated at this point > >> > >call > >> > > to is_global_var (decl) on global variable return false. > >> > > > >> > > $ cat foo.c > >> > > long long x; > >> > > int main() > >> > > { > >> > > if (__alignof__(x) != 8) > >> > >__builtin_abort(); > >> > > return 0; > >> > > } > >> > > > >> > > Breakpoint 1, layout_decl (decl=0x77ffbb40, known_align=0) > >> > > at /local/skpandey/gccwork/gccwork/gcc/gcc/stor-layout.c:674 > >> > > 674 do_type_align (type, decl); > >> > > Missing separate debuginfos, use: dnf d
Re: [PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237]
On Mon, Jul 20, 2020 at 5:06 AM Richard Biener wrote: > > On Sat, Jul 18, 2020 at 7:57 AM Sunil Pandey wrote: > > > > On Fri, Jul 17, 2020 at 1:22 AM Richard Biener > > wrote: > > > > > > On Fri, Jul 17, 2020 at 7:15 AM Sunil Pandey wrote: > > > > > > > > Any comment on revised patch? At least, in finish_decl, decl global > > > > attributes are populated. > > > > > > +static void > > > +ix86_lower_local_decl_alignment (tree decl) > > > +{ > > > + unsigned new_align = LOCAL_DECL_ALIGNMENT (decl); > > > > > > please use the macro-expanded call here since we want to amend > > > ix86_local_alignment to _not_ return a lower alignment when > > > called as LOCAL_DECL_ALIGNMENT (by adding a new parameter > > > to ix86_local_alignment). Can you also amend the patch in this > > > way? > > > > > > + if (new_align < DECL_ALIGN (decl)) > > > +SET_DECL_ALIGN (decl, new_align); > > > > > > diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c > > > index 81bd2ee94f0..1ae99e30ed1 100644 > > > --- a/gcc/c/c-decl.c > > > +++ b/gcc/c/c-decl.c > > > @@ -5601,6 +5601,8 @@ finish_decl (tree decl, location_t init_loc, tree > > > init, > > > } > > > > > >invoke_plugin_callbacks (PLUGIN_FINISH_DECL, decl); > > > + /* Lower local decl alignment. */ > > > + lower_decl_alignment (decl); > > > } > > > > > > should come before plugin hook invocation, likewise for the > > > cp_finish_decl case. > > > > > > +/* Lower DECL alignment. */ > > > + > > > +void > > > +lower_decl_alignment (tree decl) > > > +{ > > > + if (VAR_P (decl) > > > + && !is_global_var (decl) > > > + && !DECL_HARD_REGISTER (decl)) > > > +targetm.lower_local_decl_alignment (decl); > > > +} > > > > > > please avoid this function, it's name sounds too generic and it's not > > > worth > > > adding a public API for two calls. > > > > > > Alltogether this should avoid the x86 issue leaving left-overs (your > > > identified > > > inliner case) as missed optimization [for the linux kernel which > > > appearantly > > > decided that -mpreferred-stack-boundary=2 is a good ABI to use]. > > > > > > Richard. > > > > > > > > Revised patch attached. > > @@ -16776,7 +16783,7 @@ ix86_data_alignment (tree type, unsigned int > align, bool opt) > > unsigned int > ix86_local_alignment (tree exp, machine_mode mode, > - unsigned int align) > + unsigned int align, bool setalign) > { >tree type, decl; > > @@ -16801,6 +16808,10 @@ ix86_local_alignment (tree exp, machine_mode mode, >&& (!decl || !DECL_USER_ALIGN (decl))) > align = 32; > > + /* Lower decl alignment. */ > + if (setalign && align < DECL_ALIGN (decl)) > +SET_DECL_ALIGN (decl, align); > + >/* If TYPE is NULL, we are allocating a stack slot for caller-save > register in MODE. We will return the largest alignment of XF > and DF. */ > > sorry for not being clear - the parameter should indicate whether an > alignment lower > than natural alignment is OK to return thus sth like > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > index 31757b044c8..19703cbceb9 100644 > --- a/gcc/config/i386/i386.c > +++ b/gcc/config/i386/i386.c > @@ -16641,7 +16641,7 @@ ix86_data_alignment (tree type, unsigned int > align, bool opt) > > unsigned int > ix86_local_alignment (tree exp, machine_mode mode, > - unsigned int align) > + unsigned int align, bool may_lower) > { >tree type, decl; > > @@ -16658,7 +16658,8 @@ ix86_local_alignment (tree exp, machine_mode mode, > >/* Don't do dynamic stack realignment for long long objects with > -mpreferred-stack-boundary=2. */ > - if (!TARGET_64BIT > + if (may_lower > + && !TARGET_64BIT >&& align == 64 >&& ix86_preferred_stack_boundary < 64 >&& (mode == DImode || (type && TYPE_MODE (type) == DImode)) > > I also believe that spill_slot_alignment () should be able to get the > lower alignment > for long long but not get_stack_local_alignment () (both use > STACK_SLOT_ALIGNMENT). > Some uses of STACK_SLOT_ALIGNMENT also look fishy with respect to mem > attributes > and alignment. > > Otherwise the patch looks reasonable to salvage a misguided optimization for > a non-standard ABI. If it is sufficient to make the people using that ABI > happy > is of course another question. I'd rather see them stop using it ... > > That said, I'm hesitant to be the only one OKing this ugliness but I'd > immediately > OK a patch removing the questionable hunk from ix86_local_alignment ;) > > Jakub, Jeff - any opinion? > > Richard. > Revised patch attached. > > > > On Tue, Jul 14, 2020 at 8:37 AM Sunil Pandey wrote: > > > >> > > > >> On Sat, Jul 4, 2020 at 9:11 AM Richard Biener > > > >> wrote: > > > >> > > > > >> > On July 3, 2020 11:16:46 PM GMT+02:00, Jason Merrill > > > >> > wrote: > > > >> > >On 6/29/20 5:00 AM, Richard Biener wrote: > > > >> > >> On Fri, Jun 26, 2020 at 10:11 PM H.J. Lu > > > >> > >> w
Re: [PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237]
On Tue, Jul 21, 2020 at 12:50 AM Richard Biener wrote: > > On Tue, Jul 21, 2020 at 7:16 AM Sunil Pandey wrote: > > > > On Mon, Jul 20, 2020 at 5:06 AM Richard Biener > > wrote: > > > > > > On Sat, Jul 18, 2020 at 7:57 AM Sunil Pandey wrote: > > > > > > > > On Fri, Jul 17, 2020 at 1:22 AM Richard Biener > > > > wrote: > > > > > > > > > > On Fri, Jul 17, 2020 at 7:15 AM Sunil Pandey > > > > > wrote: > > > > > > > > > > > > Any comment on revised patch? At least, in finish_decl, decl > > > > > > global attributes are populated. > > > > > > > > > > +static void > > > > > +ix86_lower_local_decl_alignment (tree decl) > > > > > +{ > > > > > + unsigned new_align = LOCAL_DECL_ALIGNMENT (decl); > > > > > > > > > > please use the macro-expanded call here since we want to amend > > > > > ix86_local_alignment to _not_ return a lower alignment when > > > > > called as LOCAL_DECL_ALIGNMENT (by adding a new parameter > > > > > to ix86_local_alignment). Can you also amend the patch in this > > > > > way? > > > > > > > > > > + if (new_align < DECL_ALIGN (decl)) > > > > > +SET_DECL_ALIGN (decl, new_align); > > > > > > > > > > diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c > > > > > index 81bd2ee94f0..1ae99e30ed1 100644 > > > > > --- a/gcc/c/c-decl.c > > > > > +++ b/gcc/c/c-decl.c > > > > > @@ -5601,6 +5601,8 @@ finish_decl (tree decl, location_t init_loc, > > > > > tree init, > > > > > } > > > > > > > > > >invoke_plugin_callbacks (PLUGIN_FINISH_DECL, decl); > > > > > + /* Lower local decl alignment. */ > > > > > + lower_decl_alignment (decl); > > > > > } > > > > > > > > > > should come before plugin hook invocation, likewise for the > > > > > cp_finish_decl case. > > > > > > > > > > +/* Lower DECL alignment. */ > > > > > + > > > > > +void > > > > > +lower_decl_alignment (tree decl) > > > > > +{ > > > > > + if (VAR_P (decl) > > > > > + && !is_global_var (decl) > > > > > + && !DECL_HARD_REGISTER (decl)) > > > > > +targetm.lower_local_decl_alignment (decl); > > > > > +} > > > > > > > > > > please avoid this function, it's name sounds too generic and it's not > > > > > worth > > > > > adding a public API for two calls. > > > > > > > > > > Alltogether this should avoid the x86 issue leaving left-overs (your > > > > > identified > > > > > inliner case) as missed optimization [for the linux kernel which > > > > > appearantly > > > > > decided that -mpreferred-stack-boundary=2 is a good ABI to use]. > > > > > > > > > > Richard. > > > > > > > > > > > > > > Revised patch attached. > > > > > > @@ -16776,7 +16783,7 @@ ix86_data_alignment (tree type, unsigned int > > > align, bool opt) > > > > > > unsigned int > > > ix86_local_alignment (tree exp, machine_mode mode, > > > - unsigned int align) > > > + unsigned int align, bool setalign) > > > { > > >tree type, decl; > > > > > > @@ -16801,6 +16808,10 @@ ix86_local_alignment (tree exp, machine_mode > > > mode, > > >&& (!decl || !DECL_USER_ALIGN (decl))) > > > align = 32; > > > > > > + /* Lower decl alignment. */ > > > + if (setalign && align < DECL_ALIGN (decl)) > > > +SET_DECL_ALIGN (decl, align); > > > + > > >/* If TYPE is NULL, we are allocating a stack slot for caller-save > > > register in MODE. We will return the largest alignment of XF > > > and DF. */ > > > > > > sorry for not being clear - the parameter should indicate whether an > > > alignment lower > > > than natural alignment is OK to return thus sth like > > > > > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > > > index 31757b044c8..19703cbceb9 100644 > > > --- a/gcc/config/i386/i386.c > > > +++ b/gcc/config/i386/i386.c > > > @@ -16641,7 +16641,7 @@ ix86_data_alignment (tree type, unsigned int > > > align, bool opt) > > > > > > unsigned int > > > ix86_local_alignment (tree exp, machine_mode mode, > > > - unsigned int align) > > > + unsigned int align, bool may_lower) > > > { > > >tree type, decl; > > > > > > @@ -16658,7 +16658,8 @@ ix86_local_alignment (tree exp, machine_mode mode, > > > > > >/* Don't do dynamic stack realignment for long long objects with > > > -mpreferred-stack-boundary=2. */ > > > - if (!TARGET_64BIT > > > + if (may_lower > > > + && !TARGET_64BIT > > >&& align == 64 > > >&& ix86_preferred_stack_boundary < 64 > > >&& (mode == DImode || (type && TYPE_MODE (type) == DImode)) > > > > > > I also believe that spill_slot_alignment () should be able to get the > > > lower alignment > > > for long long but not get_stack_local_alignment () (both use > > > STACK_SLOT_ALIGNMENT). > > > Some uses of STACK_SLOT_ALIGNMENT also look fishy with respect to mem > > > attributes > > > and alignment. > > > > > > Otherwise the patch looks reasonable to salvage a misguided optimization > > > for > > > a non-standard ABI. If it is sufficient to make the people using that > > > ABI happy > >
Re: [PATCH] Add TARGET_UPDATE_DECL_ALIGNMENT [PR95237]
On Wed, Jun 24, 2020 at 12:30 AM Richard Biener wrote: > > On Tue, Jun 23, 2020 at 5:31 PM Sunil K Pandey via Gcc-patches > wrote: > > > > From: Sunil K Pandey > > > > Default for this hook is NOP. For x86, in 32 bit mode, this hook > > sets alignment of long long on stack to 32 bits if preferred stack > > boundary is 32 bits. > > > > - This patch fixes > > gcc.target/i386/pr69454-2.c > > gcc.target/i386/stackalign/longlong-1.c > > - Regression test on x86-64, no new fail introduced. > > I think the name is badly chosen, TARGET_LOWER_LOCAL_DECL_ALIGNMENT Yes, I can change the target hook name. > would be better suited (and then asks for LOCAL_DECL_ALIGNMENT to be > renamed to INCREASE_LOCAL_DECL_ALIGNMENT). It seems like LOCAL_DECL_ALIGNMENT macro documentation is incorrect. It increases as well as decreases alignment based on condition(-m32 -mpreferred-stack-boundary=2) https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95885 > > You're calling it from do_type_align which IMHO is dangerous since that's > invoked from FIELD_DECL layout as well. Instead invoke it from > layout_decl itself where we do > > if (code != FIELD_DECL) > /* For non-fields, update the alignment from the type. */ > do_type_align (type, decl); > > and invoke the hook _after_ do_type_align. Also avoid > invoking the hook on globals or hard regs and only > invoke it on VAR_DECLs, thus only > > if (VAR_P (decl) && !is_global_var (decl) && !DECL_HARD_REGISTER (decl)) It seems like decl property is not fully populated at this point call to is_global_var (decl) on global variable return false. $ cat foo.c long long x; int main() { if (__alignof__(x) != 8) __builtin_abort(); return 0; } Breakpoint 1, layout_decl (decl=0x77ffbb40, known_align=0) at /local/skpandey/gccwork/gccwork/gcc/gcc/stor-layout.c:674 674 do_type_align (type, decl); Missing separate debuginfos, use: dnf debuginfo-install gmp-6.1.2-10.fc31.x86_64 isl-0.16.1-9.fc31.x86_64 libmpc-1.1.0-4.fc31.x86_64 mpfr-3.1.6-5.fc31.x86_64 zlib-1.2.11-20.fc31.x86_64 (gdb) call debug_tree(decl) unit-size align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7fffea801888 precision:64 min max pointer_to_this > DI foo.c:1:11 size unit-size align:1 warn_if_not_align:0> (gdb) p is_global_var(decl) $1 = false (gdb) What about calling hook here 603 do_type_align (tree type, tree decl) 604 { 605 if (TYPE_ALIGN (type) > DECL_ALIGN (decl)) 606 { 607 SET_DECL_ALIGN (decl, TYPE_ALIGN (type)); 608 if (TREE_CODE (decl) == FIELD_DECL) 609 DECL_USER_ALIGN (decl) = TYPE_USER_ALIGN (type); 610 else 611 /* Lower local decl alignment */ 612 if (VAR_P (decl) 613 && !is_global_var (decl) 614 && !DECL_HARD_REGISTER (decl) 615 && cfun != NULL) 616 targetm.lower_local_decl_alignment (decl); 617 } > > Comments on the hook itself below. > > > Tested on x86-64. > > > > gcc/ChangeLog: > > > > PR target/95237 > > * config/i386/i386.c (ix86_update_decl_alignment): New > > function. > > (TARGET_UPDATE_DECL_ALIGNMENT): Define. > > * doc/tm.texi: Regenerate. > > * doc/tm.texi.in (TARGET_UPDATE_DECL_ALIGNMENT): New hook. > > * stor-layout.c (do_type_align): Call target hook to update > > decl alignment. > > * target.def (update_decl_alignment): New hook. > > > > gcc/testsuite/ChangeLog: > > > > PR target/95237 > > * gcc.target/i386/pr95237-1.c: New test. > > * gcc.target/i386/pr95237-2.c: New test. > > * gcc.target/i386/pr95237-3.c: New test. > > * gcc.target/i386/pr95237-4.c: New test. > > * gcc.target/i386/pr95237-5.c: New test. > > --- > > gcc/config/i386/i386.c| 22 ++ > > gcc/doc/tm.texi | 5 + > > gcc/doc/tm.texi.in| 2 ++ > > gcc/stor-layout.c | 2 ++ > > gcc/target.def| 7 +++ > > gcc/testsuite/gcc.target/i386/pr95237-1.c | 16 > > gcc/testsuite/gcc.target/i386/pr95237-2.c | 10 ++ > > gcc/testsuite/gcc.target/i386/pr95237-3.c | 10 ++ > > gcc/testsuite/gcc.target/i386/pr95237-4.c | 10 ++ > > gcc/testsuite/gcc.target/i386/pr95237-5.c | 16 > > 10 files changed, 100 insertions(+) > > create mode 100644 gcc/testsuite/gcc.target/i386/pr95237-1.c > > create mode 100644 gcc/testsuite/gcc.target/i386/pr95237-2.c > > create mode 100644 gcc/testsuite/gcc.target/i386/pr95237-3.c > > create mode 100644 gcc/testsuite/gcc.target/i386/pr95237-4.c > > create mode 100644 gcc/testsuite/gcc.target/i386/pr95237-5.c > > > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > > index 37aaa49996d..bcd9abd5303 100644 > > --- a/gcc/config/i386/i386.c > > +++ b/gcc/config/i386/i38