PR117350: Keep assembler name for abstract decls for autofdo
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Test passed
|
Commit Message
From: Andi Kleen <ak@gcc.gnu.org>
autofdo looks up inline stacks and tries to match them with the profile
data using their symbol name. Make sure all decls that can be in a inline stack
have a valid assembler name.
This fixes a bootstrap problem with autoprofiledbootstrap and LTO.
2024-10-30 Jason Merrill <jason@redhat.com>
Andrew Pinski <quic_apinski@quicinc.com>
Andi Kleen <ak@gcc.gnu.org>
gcc/ChangeLog:
PR bootstrap/117350
* tree.cc (need_assembler_name_p): Keep assembler name
for abstract declarations when autofdo is used.
---
gcc/tree.cc | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Comments
On 10/31/24 4:40 PM, Andi Kleen wrote:
> From: Andi Kleen <ak@gcc.gnu.org>
>
> autofdo looks up inline stacks and tries to match them with the profile
> data using their symbol name. Make sure all decls that can be in a inline stack
> have a valid assembler name.
>
> This fixes a bootstrap problem with autoprofiledbootstrap and LTO.
OK in a week if no other comments.
> 2024-10-30 Jason Merrill <jason@redhat.com>
> Andrew Pinski <quic_apinski@quicinc.com>
> Andi Kleen <ak@gcc.gnu.org>
> gcc/ChangeLog:
>
> PR bootstrap/117350
> * tree.cc (need_assembler_name_p): Keep assembler name
> for abstract declarations when autofdo is used.
> ---
> gcc/tree.cc | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/tree.cc b/gcc/tree.cc
> index b4c059d3b0db..92f99eaccd72 100644
> --- a/gcc/tree.cc
> +++ b/gcc/tree.cc
> @@ -787,8 +787,9 @@ need_assembler_name_p (tree decl)
> || DECL_ASSEMBLER_NAME_SET_P (decl))
> return false;
>
> - /* Abstract decls do not need an assembler name. */
> - if (DECL_ABSTRACT_P (decl))
> + /* Abstract decls do not need an assembler name, except they
> + can be looked up by autofdo. */
> + if (DECL_ABSTRACT_P (decl) && !flag_auto_profile)
> return false;
>
> /* For VAR_DECLs, only static, public and external symbols need an
On Tue, Nov 5, 2024 at 2:02 AM Jason Merrill <jason@redhat.com> wrote:
>
> On 10/31/24 4:40 PM, Andi Kleen wrote:
> > From: Andi Kleen <ak@gcc.gnu.org>
> >
> > autofdo looks up inline stacks and tries to match them with the profile
> > data using their symbol name. Make sure all decls that can be in a inline stack
> > have a valid assembler name.
> >
> > This fixes a bootstrap problem with autoprofiledbootstrap and LTO.
>
> OK in a week if no other comments.
Hmm, but DECL_ABSTRACT_P should be only set on entities that generate no code.
How does autofdo look them up? Are you sure it's the abstract decl
autofdo wants
to lookup? Or is autofdo processing not serializing the compilation and thus it
affects code generation on parts that have not been processed yet?
That said, the patch doesn't make much sense to me.
Richard.
> > 2024-10-30 Jason Merrill <jason@redhat.com>
> > Andrew Pinski <quic_apinski@quicinc.com>
> > Andi Kleen <ak@gcc.gnu.org>
> > gcc/ChangeLog:
> >
> > PR bootstrap/117350
> > * tree.cc (need_assembler_name_p): Keep assembler name
> > for abstract declarations when autofdo is used.
> > ---
> > gcc/tree.cc | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/gcc/tree.cc b/gcc/tree.cc
> > index b4c059d3b0db..92f99eaccd72 100644
> > --- a/gcc/tree.cc
> > +++ b/gcc/tree.cc
> > @@ -787,8 +787,9 @@ need_assembler_name_p (tree decl)
> > || DECL_ASSEMBLER_NAME_SET_P (decl))
> > return false;
> >
> > - /* Abstract decls do not need an assembler name. */
> > - if (DECL_ABSTRACT_P (decl))
> > + /* Abstract decls do not need an assembler name, except they
> > + can be looked up by autofdo. */
> > + if (DECL_ABSTRACT_P (decl) && !flag_auto_profile)
> > return false;
> >
> > /* For VAR_DECLs, only static, public and external symbols need an
>
On Tue, Nov 05, 2024 at 09:47:17AM +0100, Richard Biener wrote:
> On Tue, Nov 5, 2024 at 2:02 AM Jason Merrill <jason@redhat.com> wrote:
> >
> > On 10/31/24 4:40 PM, Andi Kleen wrote:
> > > From: Andi Kleen <ak@gcc.gnu.org>
> > >
> > > autofdo looks up inline stacks and tries to match them with the profile
> > > data using their symbol name. Make sure all decls that can be in a inline stack
> > > have a valid assembler name.
> > >
> > > This fixes a bootstrap problem with autoprofiledbootstrap and LTO.
> >
> > OK in a week if no other comments.
>
> Hmm, but DECL_ABSTRACT_P should be only set on entities that generate no code.
>
> How does autofdo look them up? Are you sure it's the abstract decl
> autofdo wants
> to lookup? Or is autofdo processing not serializing the compilation and thus it
> affects code generation on parts that have not been processed yet?
autofdo tries to match inlines to an inline stack derived from dwarf.
So if something appears in dwarf it has to be in its stack. For the
test case the abstract entity is in the dwarf stack.
For the inside gcc lookup it walks the BLOCK_SUPERCONTEXT links
and looks at BLOCK_ABSTRACT_ORIGIN and ignores everything that has unknown
location
Maybe there could be some filtering there, but it would need to be on
both sides, which would be a version break for the file format.
-Andi
On Tue, Nov 5, 2024 at 6:08 PM Andi Kleen <andi@firstfloor.org> wrote:
>
> On Tue, Nov 05, 2024 at 09:47:17AM +0100, Richard Biener wrote:
> > On Tue, Nov 5, 2024 at 2:02 AM Jason Merrill <jason@redhat.com> wrote:
> > >
> > > On 10/31/24 4:40 PM, Andi Kleen wrote:
> > > > From: Andi Kleen <ak@gcc.gnu.org>
> > > >
> > > > autofdo looks up inline stacks and tries to match them with the profile
> > > > data using their symbol name. Make sure all decls that can be in a inline stack
> > > > have a valid assembler name.
> > > >
> > > > This fixes a bootstrap problem with autoprofiledbootstrap and LTO.
> > >
> > > OK in a week if no other comments.
> >
> > Hmm, but DECL_ABSTRACT_P should be only set on entities that generate no code.
> >
> > How does autofdo look them up? Are you sure it's the abstract decl
> > autofdo wants
> > to lookup? Or is autofdo processing not serializing the compilation and thus it
> > affects code generation on parts that have not been processed yet?
>
>
> autofdo tries to match inlines to an inline stack derived from dwarf.
> So if something appears in dwarf it has to be in its stack. For the
> test case the abstract entity is in the dwarf stack.
>
> For the inside gcc lookup it walks the BLOCK_SUPERCONTEXT links
> and looks at BLOCK_ABSTRACT_ORIGIN and ignores everything that has unknown
> location
>
> Maybe there could be some filtering there, but it would need to be on
> both sides, which would be a version break for the file format.
Ah, OK - I wasn't aware it uses/requires debug info. It's still a bit odd that
only the abstract decl has the correct symbol name. If it's abstract shouldn't
it have the same symbol name as an actual entry? Maybe the symbol name
noted in the debug info is wrong? I think the only DECL_ABSTRACT_P decls
we have are from C++ CTOR/DTOR stuff.
In any case, I withdraw my objection.
Thanks for explaining,
Richard.
> -Andi
>
The patch looks good to me.
-----Original Message-----
From: Richard Biener <richard.guenther@gmail.com>
Sent: Wednesday, November 6, 2024 12:01 AM
To: Andi Kleen <andi@firstfloor.org>
Cc: Jason Merrill <jason@redhat.com>; Andi Kleen <ak@linux.intel.com>; gcc-patches@gcc.gnu.org; Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com>; pinskia@gmail.com; Andi Kleen <ak@gcc.gnu.org>
Subject: [EXTERNAL] Re: [PATCH] PR117350: Keep assembler name for abstract decls for autofdo
On Tue, Nov 5, 2024 at 6:08 PM Andi Kleen <andi@firstfloor.org> wrote:
>
> On Tue, Nov 05, 2024 at 09:47:17AM +0100, Richard Biener wrote:
> > On Tue, Nov 5, 2024 at 2:02 AM Jason Merrill <jason@redhat.com> wrote:
> > >
> > > On 10/31/24 4:40 PM, Andi Kleen wrote:
> > > > From: Andi Kleen <ak@gcc.gnu.org>
> > > >
> > > > autofdo looks up inline stacks and tries to match them with the
> > > > profile data using their symbol name. Make sure all decls that
> > > > can be in a inline stack have a valid assembler name.
> > > >
> > > > This fixes a bootstrap problem with autoprofiledbootstrap and LTO.
> > >
> > > OK in a week if no other comments.
> >
> > Hmm, but DECL_ABSTRACT_P should be only set on entities that generate no code.
> >
> > How does autofdo look them up? Are you sure it's the abstract decl
> > autofdo wants to lookup? Or is autofdo processing not serializing
> > the compilation and thus it affects code generation on parts that
> > have not been processed yet?
>
>
> autofdo tries to match inlines to an inline stack derived from dwarf.
> So if something appears in dwarf it has to be in its stack. For the
> test case the abstract entity is in the dwarf stack.
>
> For the inside gcc lookup it walks the BLOCK_SUPERCONTEXT links and
> looks at BLOCK_ABSTRACT_ORIGIN and ignores everything that has unknown
> location
>
> Maybe there could be some filtering there, but it would need to be on
> both sides, which would be a version break for the file format.
Ah, OK - I wasn't aware it uses/requires debug info. It's still a bit odd that only the abstract decl has the correct symbol name. If it's abstract shouldn't it have the same symbol name as an actual entry? Maybe the symbol name noted in the debug info is wrong? I think the only DECL_ABSTRACT_P decls we have are from C++ CTOR/DTOR stuff.
In any case, I withdraw my objection.
Thanks for explaining,
Richard.
> -Andi
>
On Thu, Oct 31, 2024 at 1:41 PM Andi Kleen <ak@linux.intel.com> wrote:
>
> From: Andi Kleen <ak@gcc.gnu.org>
>
> autofdo looks up inline stacks and tries to match them with the profile
> data using their symbol name. Make sure all decls that can be in a inline stack
> have a valid assembler name.
>
> This fixes a bootstrap problem with autoprofiledbootstrap and LTO.
>
> 2024-10-30 Jason Merrill <jason@redhat.com>
> Andrew Pinski <quic_apinski@quicinc.com>
> Andi Kleen <ak@gcc.gnu.org>
> gcc/ChangeLog:
>
> PR bootstrap/117350
> * tree.cc (need_assembler_name_p): Keep assembler name
> for abstract declarations when autofdo is used.
Looks like this patch was never pushed even though it was approved and
the bug report was closed as being fixed.
I suspect https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117782 is a
duplicate of bug 117350 now too.
Thanks,
Andrew
> ---
> gcc/tree.cc | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/tree.cc b/gcc/tree.cc
> index b4c059d3b0db..92f99eaccd72 100644
> --- a/gcc/tree.cc
> +++ b/gcc/tree.cc
> @@ -787,8 +787,9 @@ need_assembler_name_p (tree decl)
> || DECL_ASSEMBLER_NAME_SET_P (decl))
> return false;
>
> - /* Abstract decls do not need an assembler name. */
> - if (DECL_ABSTRACT_P (decl))
> + /* Abstract decls do not need an assembler name, except they
> + can be looked up by autofdo. */
> + if (DECL_ABSTRACT_P (decl) && !flag_auto_profile)
> return false;
>
> /* For VAR_DECLs, only static, public and external symbols need an
> --
> 2.46.2
>
On Tue, Nov 26, 2024 at 04:06:37PM -0800, Andrew Pinski wrote:
> On Thu, Oct 31, 2024 at 1:41 PM Andi Kleen <ak@linux.intel.com> wrote:
> >
> > From: Andi Kleen <ak@gcc.gnu.org>
> >
> > autofdo looks up inline stacks and tries to match them with the profile
> > data using their symbol name. Make sure all decls that can be in a inline stack
> > have a valid assembler name.
> >
> > This fixes a bootstrap problem with autoprofiledbootstrap and LTO.
> >
> > 2024-10-30 Jason Merrill <jason@redhat.com>
> > Andrew Pinski <quic_apinski@quicinc.com>
> > Andi Kleen <ak@gcc.gnu.org>
> > gcc/ChangeLog:
> >
> > PR bootstrap/117350
> > * tree.cc (need_assembler_name_p): Keep assembler name
> > for abstract declarations when autofdo is used.
>
>
> Looks like this patch was never pushed even though it was approved and
> the bug report was closed as being fixed.
> I suspect https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117782 is a
> duplicate of bug 117350 now too.
Oops. Will do it it now. Thanks.
-Andi
@@ -787,8 +787,9 @@ need_assembler_name_p (tree decl)
|| DECL_ASSEMBLER_NAME_SET_P (decl))
return false;
- /* Abstract decls do not need an assembler name. */
- if (DECL_ABSTRACT_P (decl))
+ /* Abstract decls do not need an assembler name, except they
+ can be looked up by autofdo. */
+ if (DECL_ABSTRACT_P (decl) && !flag_auto_profile)
return false;
/* For VAR_DECLs, only static, public and external symbols need an