public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jeff Law <jeffreyalaw@gmail.com>
To: "Jose E. Marchesi" <jose.marchesi@oracle.com>,
	"Jose E. Marchesi via Gcc-patches" <gcc-patches@gcc.gnu.org>,
	Jakub Jelinek <jakub@redhat.com>,
	richard.sandiford@arm.com
Subject: Re: [PATCH V2] Emit funcall external declarations only if actually used.
Date: Thu, 5 Oct 2023 16:37:31 -0600	[thread overview]
Message-ID: <d0f7d7c6-5678-41d1-bee4-34f3a96cd503@gmail.com> (raw)
In-Reply-To: <mpto7hclnnv.fsf@arm.com>



On 10/5/23 16:17, Richard Sandiford wrote:
> "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.
> 
> 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.
It might be worth looking at the PA port.  There's a fair number of 
similarities.  Essentially it has to "import" every external symbol, 
including libcall symbols.   A symbol which has been exported, can not 
then also be imported as that can change the underlying type.

IIRC we implemented all the magic to handle this inside 
ASM_OUTPUT_EXTERNAL and ASM_OUTPUT_EXTERNAL_LIBCALL with a bit of magic 
to handle "plabel" symbols which we have to defer until the end of the 
compilation unit via pa_end_file or whatever it's called since some 
cases of symbol marking couldn't be determined until we were actually 
emitting code.

jeff


  reply	other threads:[~2023-10-05 22:37 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 [this message]
2023-10-12 11:38       ` Jose E. Marchesi
2023-10-12 12:49         ` Richard Sandiford
2023-10-12 13:54           ` Jose E. Marchesi
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=d0f7d7c6-5678-41d1-bee4-34f3a96cd503@gmail.com \
    --to=jeffreyalaw@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jakub@redhat.com \
    --cc=jose.marchesi@oracle.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).