On 08/03/2021 16:21, Richard Biener wrote:
> 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?
The option of just removing the target hook seems appealing, if that
would be acceptable. It looks like avr could easily be changed to just
call its internal implementation (avr_builtin_decl) instead of the
target hook.
>
> > 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.
