public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: "Jose E. Marchesi" <jose.marchesi@oracle.com>
To: "Jose E. Marchesi via Gcc-patches" <gcc-patches@gcc.gnu.org>
Cc: Jakub Jelinek <jakub@redhat.com>, richard.sandiford@arm.com
Subject: Re: [PATCH V2] Emit funcall external declarations only if actually used.
Date: Thu, 12 Oct 2023 15:54:47 +0200	[thread overview]
Message-ID: <87cyxkymhk.fsf@oracle.com> (raw)
In-Reply-To: <mptr0m09fad.fsf@arm.com> (Richard Sandiford's message of "Thu, 12 Oct 2023 13:49:30 +0100")


> "Jose E. Marchesi" <jose.marchesi@oracle.com> writes:
>> Hi Richard.
>> Thanks for looking at this! :)
>>
>>
>>> "Jose E. Marchesi" <jose.marchesi@oracle.com> writes:
>>>> ping
>>>
>>> I don't know this code very well, and have AFAIR haven't worked
>>> with an assembler that requires external declarations, but since
>>> it's at a second ping :)
>>>
>>>>
>>>>> ping
>>>>>
>>>>>> [Differences from V1:
>>>>>> - Prototype for call_from_call_insn moved before comment block.
>>>>>> - Reuse the `call' flag for SYMBOL_REF_LIBCALL.
>>>>>> - Fallback to check REG_CALL_DECL in non-direct calls.
>>>>>> - New test to check correct behavior for non-direct calls.]
>>>>>>
>>>>>> There are many places in GCC where alternative local sequences are
>>>>>> tried in order to determine what is the cheapest or best alternative
>>>>>> to use in the current target.  When any of these sequences involve a
>>>>>> libcall, the current implementation of emit_library_call_value_1
>>>>>> introduce a side-effect consisting on emitting an external declaration
>>>>>> for the funcall (such as __divdi3) which is thus emitted even if the
>>>>>> sequence that does the libcall is not retained.
>>>>>>
>>>>>> This is problematic in targets such as BPF, because the kernel loader
>>>>>> chokes on the spurious symbol __divdi3 and makes the resulting BPF
>>>>>> object unloadable.  Note that BPF objects are not linked before being
>>>>>> loaded.
>>>>>>
>>>>>> This patch changes emit_library_call_value_1 to mark the target
>>>>>> SYMBOL_REF as a libcall.  Then, the emission of the external
>>>>>> declaration is done in the first loop of final.cc:shorten_branches.
>>>>>> This happens only if the corresponding sequence has been kept.
>>>>>>
>>>>>> Regtested in x86_64-linux-gnu.
>>>>>> Tested with host x86_64-linux-gnu with target bpf-unknown-none.
>>>
>>> I'm not sure that shorten_branches is a natural place to do this.
>>> It isn't something that would normally emit asm text.
>>
>> Well, that was the approach suggested by another reviewer (Jakub) once
>> my initial approach (in the V1) got rejected.  He explicitly suggested
>> to use shorten_branches.
>>
>>> Would it be OK to emit the declaration at the same point as for decls,
>>> which IIUC is process_pending_assemble_externals?  If so, how about
>>> making assemble_external_libcall add the symbol to a list when
>>> !SYMBOL_REF_USED, instead of calling targetm.asm_out.external_libcall
>>> directly?  assemble_external_libcall could then also call get_identifier
>>> on the name (perhaps after calling strip_name_encoding -- can't
>>> remember whether assemble_external_libcall sees the encoded or
>>> unencoded name).
>>>
>>> All being well, the call to get_identifier should cause
>>> assemble_name_resolve to record when the name is used, via
>>> TREE_SYMBOL_REFERENCED.  Then process_pending_assemble_externals could
>>> go through the list of libcalls recorded by assemble_external_libcall
>>> and check whether TREE_SYMBOL_REFERENCED is set on the get_identifier.
>>>
>>> Not super elegant, but it seems to fit within the existing scheme.
>>> And I don't there should be any problem with using get_identifier
>>> for libcalls, since it isn't valid to use libcall names for other
>>> types of symbol.
>>
>> This sounds way more complicated to me than the approach in V2, which
>> seems to work and is thus a clear improvement compared to the current
>> situation in the trunk.  The approach in V2 may be ugly, but it is
>> simple and easy to understand.  Is the proposed more convoluted
>> alternative really worth the extra complexity, given it is "not super
>> elegant"?
>
> Is it really that much more convoluted?  I was thinking of something
> like the attached, which seems a bit shorter than V2, and does seem
> to fix the bpf tests.

o_O
Ok I clearly misunderstood what you was proposing.  This is way simpler!

How does the magic of TREE_SYMBOL_REFERENCED work?  How is it set to
`true' only if the RTL containing the call is retained in the final
chain?

> I think most (all?) libcalls already have an associated decl due to
> optabs-libfuncs.cc, so an alternative to get_identifier would be to
> set the SYMBOL_REF_DECL.  Using get_identiifer seems a bit more
> lightweight though.
>
> Richard
>
> diff --git a/gcc/varasm.cc b/gcc/varasm.cc
> index b0eff17b8b5..073e3eb2579 100644
> --- a/gcc/varasm.cc
> +++ b/gcc/varasm.cc
> @@ -2461,6 +2461,10 @@ contains_pointers_p (tree type)
>     it all the way to final.  See PR 17982 for further discussion.  */
>  static GTY(()) tree pending_assemble_externals;
>  
> +/* A similar list of pending libcall symbols.  We only want to declare
> +   symbols that are actually used in the final assembly.  */
> +static GTY(()) rtx pending_libcall_symbols;
> +
>  #ifdef ASM_OUTPUT_EXTERNAL
>  /* Some targets delay some output to final using TARGET_ASM_FILE_END.
>     As a result, assemble_external can be called after the list of externals
> @@ -2516,12 +2520,20 @@ void
>  process_pending_assemble_externals (void)
>  {
>  #ifdef ASM_OUTPUT_EXTERNAL
> -  tree list;
> -  for (list = pending_assemble_externals; list; list = TREE_CHAIN (list))
> +  for (tree list = pending_assemble_externals; list; list = TREE_CHAIN (list))
>      assemble_external_real (TREE_VALUE (list));
>  
> +  for (rtx list = pending_libcall_symbols; list; list = XEXP (list, 1))
> +    {
> +      rtx symbol = XEXP (list, 0);
> +      tree id = get_identifier (XSTR (symbol, 0));
> +      if (TREE_SYMBOL_REFERENCED (id))
> +	targetm.asm_out.external_libcall (symbol);
> +    }
> +
>    pending_assemble_externals = 0;
>    pending_assemble_externals_processed = true;
> +  pending_libcall_symbols = NULL_RTX;
>    delete pending_assemble_externals_set;
>  #endif
>  }
> @@ -2594,8 +2606,11 @@ assemble_external_libcall (rtx fun)
>    /* Declare library function name external when first used, if nec.  */
>    if (! SYMBOL_REF_USED (fun))
>      {
> +      gcc_assert (!pending_assemble_externals_processed);
>        SYMBOL_REF_USED (fun) = 1;
> -      targetm.asm_out.external_libcall (fun);
> +      get_identifier (XSTR (fun, 0));
> +      pending_libcall_symbols = gen_rtx_EXPR_LIST (VOIDmode, fun,
> +						   pending_libcall_symbols);
>      }
>  }

  reply	other threads:[~2023-10-12 13:54 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-21 18:07 Jose E. Marchesi
2023-08-30  8:12 ` Jose E. Marchesi
2023-09-05 13:03   ` Jose E. Marchesi
2023-10-03 10:39   ` Jose E. Marchesi
2023-10-05 22:17     ` Richard Sandiford
2023-10-05 22:37       ` Jeff Law
2023-10-12 11:38       ` Jose E. Marchesi
2023-10-12 12:49         ` Richard Sandiford
2023-10-12 13:54           ` Jose E. Marchesi [this message]
2023-10-12 14:05             ` Richard Sandiford

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87cyxkymhk.fsf@oracle.com \
    --to=jose.marchesi@oracle.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jakub@redhat.com \
    --cc=richard.sandiford@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).