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)

2020-10-04 Thread Sunil Pandey via Gcc-patches
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

2021-05-24 Thread Sunil Pandey via Gcc-patches
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

2020-09-11 Thread Sunil Pandey via Gcc-patches
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]

2020-07-14 Thread Sunil Pandey via Gcc-patches
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]

2020-07-16 Thread Sunil Pandey via Gcc-patches
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]

2020-07-17 Thread Sunil Pandey via Gcc-patches
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]

2020-07-20 Thread Sunil Pandey via Gcc-patches
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]

2020-07-21 Thread Sunil Pandey via Gcc-patches
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]

2020-06-24 Thread Sunil Pandey via Gcc-patches
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