On Mon, Mar 8, 2021 at 4:14 PM Alex Coplan <alex.cop...@arm.com> wrote:
>
> On 08/03/2021 14:57, Richard Biener wrote:
> > On Mon, Mar 8, 2021 at 12:44 PM Alex Coplan via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > Hi all,
> > >
> > > As discussed in the PR, we currently have two different numbering
> > > schemes for SVE builtins: one for C, and one for C++. This is
> > > problematic for LTO, where we end up getting confused about which
> > > intrinsic we're talking about. This patch inserts placeholders into the
> > > registered_functions vector to ensure that there is a consistent
> > > numbering scheme for both C and C++.
> > >
> > > Initially I tried using a null decl for these placeholders, but this
> > > trips up some validation when the builtin trees are streamed into lto1,
> >
> > Care to elaborate? Note we stream builtins by their function code
> > and thus expect to be able to materialize them later - materializing
> > to NULL obviously fails but then the issue is the proper decl isn't there
> > in that case. Materializing a "dummy" decl just will delay the inevitable
> > breakage.
>
> So the validation I was referring to is this part of
> tree-streamer-in.c:unpack_ts_function_decl_value_fields:
>
> else if (cl == BUILT_IN_MD)
> {
> tree result = targetm.builtin_decl (fcode, true);
> if (!result || result == error_mark_node)
> fatal_error (input_location, "target specific builtin not available");
> }
>
> Because we stream the original decl and reconstruct it, I was assuming
> that we would be using this (real) node everywhere and we would never
> need to map from function code to tree node. Indeed, grepping for
> "targetm.builtin_decl", the only results are this use, plus a few uses
> in the avr backend, so it seems that right now this doesn't matter for
> AArch64 (unless I'm missing some other path).
Hmm, I thought we were streaming those by reference - indeed
this probably got changed when we stopped doing that for
the middle-end builtins which we did because those are often
"misrecognized".
> However, it seems that returning a subtly incorrect result for this
> taget hook is asking for trouble, and we should return the real node
> here.
Yeah, the intent of this target hook is exactly that. There's now of course
the option to simply remove it if it just exists for the checking done above
(need to check the avr backend, of course). I suppose the cost in the
LTO IR stream isn't so big thus we can safely ditch the optimization
forever?
> Richard, do you have a feeling for how we should do this? Perhaps
> instantiate real nodes (instead of placeholders) if in_lto_p? Or
> unconditionally?
>
> Thanks,
> Alex
>
> >
> > > so I ended up building dummy FUNCTION_DECLs as placeholders.
> > >
> > > To get better test coverage here, I'm wondering if we could make some of
> > > the existing ACLE tests also run with LTO? Perhaps as a follow-on patch?
> > > For now I've just added the specific testcase from the PR.
> > >
> > > Bootstrapped and regtested on aarch64-linux-gnu, OK for trunk?
> > >
> > > Thanks,
> > > Alex
> > >
> > > ---
> > >
> > > gcc/ChangeLog:
> > >
> > > PR target/99216
> > > * config/aarch64/aarch64-sve-builtins.cc
> > > (function_builder::add_function): Add placeholder_p argument,
> > > build
> > > placeholder decls if this is set.
> > > (function_builder::add_unique_function): Instead of conditionally
> > > adding
> > > direct overloads, unconditionally add either a direct overload or
> > > a
> > > placeholder.
> > > (function_builder::add_overloaded_function): Set placeholder_p if
> > > we're
> > > using C++ overloads.
> > > (function_builder::add_overloaded_functions): Don't return early
> > > for
> > > m_direct_overloads: we need to add placeholders.
> > > * config/aarch64/aarch64-sve-builtins.h
> > > (function_builder::add_function): Add placeholder_p argument.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > PR target/99216
> > > * g++.target/aarch64/sve/pr99216.C: New test.
