Stream: git-wasmtime

Topic: wasmtime / PR #5434 cranelift: Implement TLS on aarch64 M...

Wasmtime GitHub notifications bot (Dec 14 2022 at 02:44):

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 in object 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.

Wasmtime GitHub notifications bot (Dec 14 2022 at 03:01):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2022 at 03:01):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2022 at 03:01):

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 that tls_model_is_elf_gd and tls_model_is_macho are mutually exclusive, but it can distinguish the TlsModel enum arms)? Either adding rule priorities ((rule 1 ...) and (rule 0 ...)) with the if 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 :-)

Wasmtime GitHub notifications bot (Dec 14 2022 at 03:01):

cfallin created PR review comment:

uses and defs and clobbers can all be empty here, as this emission happens after regalloc runs so we don't need any of this metadata.

Wasmtime GitHub notifications bot (Dec 14 2022 at 03:01):

cfallin created PR review comment:

s/AAarch64/Aarch64/ (and in comment below too)

Wasmtime GitHub notifications bot (Dec 14 2022 at 03:23):

cfallin updated PR #5434 from aarch64-macho-support-tls to main.

Wasmtime GitHub notifications bot (Dec 15 2022 at 01:00):

nathanwhit updated PR #5434 from aarch64-macho-support-tls to main.

Wasmtime GitHub notifications bot (Dec 15 2022 at 01:19):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 01:19):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 01:19):

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 use x1 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

Wasmtime GitHub notifications bot (Dec 15 2022 at 02:20):

nathanwhit updated PR #5434 from aarch64-macho-support-tls to main.

Wasmtime GitHub notifications bot (Dec 15 2022 at 02:20):

nathanwhit submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 02:20):

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)

Wasmtime GitHub notifications bot (Feb 09 2023 at 01:42):

nathanwhit updated PR #5434 from aarch64-macho-support-tls to main.

Wasmtime GitHub notifications bot (Mar 24 2023 at 17:19):

nathanwhit updated PR #5434 from aarch64-macho-support-tls to main.

Wasmtime GitHub notifications bot (Mar 24 2023 at 18:27):

cfallin merged PR #5434.

Last updated: Nov 22 2024 at 17:03 UTC