nathanwhit opened PR #5434 from aarch64-macho-support-tls
to main
:
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Fixes #5433.This PR implements support for TLS on aarch64 Mach-O (i.e. Apple silicon). This also includes an update to
object
as this PR depends on a fix released inobject 0.30
.For testing, I've added a filetest similar to the existing test for aarch64 ELF. I wasn't sure if I should add an emit test as well, since there doesn't seem to be one for TLS on aarch64 ELF. I've also tested these changes on a local branch of cg_clif, and it passes the TLS tests there.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
The
has_type
clause shouldn't be necessary here if(if (tls_model_is_elf_gd))
below is present, or vice-versa. I'm guessing you were running into rule-overlap issues (the checker doesn't know thattls_model_is_elf_gd
andtls_model_is_macho
are mutually exclusive, but it can distinguish theTlsModel
enum arms)? Either adding rule priorities ((rule 1 ...)
and(rule 0 ...)
) with theif
clauses, or keeping the(has_type (tls_model ...))
only, would work. It looks like in x64 we did the latter so let's do that here too for consistency :-)
cfallin created PR review comment:
uses
anddefs
andclobbers
can all be empty here, as this emission happens after regalloc runs so we don't need any of this metadata.
cfallin created PR review comment:
s/AAarch64/Aarch64/ (and in comment below too)
cfallin updated PR #5434 from aarch64-macho-support-tls
to main
.
nathanwhit updated PR #5434 from aarch64-macho-support-tls
to main
.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
We probably don't want to increase the size of
Inst
if we can help it at all -- the point of this test is to guard against perf regressions. Can we find a way to remove a field or, failing that, box the contents of the inst? Two options come to mind:
- (Maybe preferred) eliminate
rtmp
, and usex1
explicitly instead. This is part of the clobber-set of the call already, so it should be legal to use it to hold the address of the called function.- (Alternately) create a struct and hold a
Box
in the inst, taking only 8 bytes
nathanwhit updated PR #5434 from aarch64-macho-support-tls
to main
.
nathanwhit submitted PR review.
nathanwhit created PR review comment:
Ah, I wasn't sure that it was okay to use
x1
, but your explanation makes sense (and helped clear up my confusion) - thanks!Just pushed a commit to make that change (and reverted the change to the size test as well)
nathanwhit updated PR #5434 from aarch64-macho-support-tls
to main
.
nathanwhit updated PR #5434 from aarch64-macho-support-tls
to main
.
cfallin merged PR #5434.
Last updated: Dec 23 2024 at 12:05 UTC