Skip to content

fix(relinker): move legacy CMPSS and CMPSD out of line for Rosetta - #5762

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
AlexOfTheForest:fix/rosetta-legacy-compare
Oct 15, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
AlexOfTheForest:fix/rosetta-legacy-compare

Conversation

@AlexOfTheForest

Copy link
Copy Markdown
Contributor

What

Follow-up to #5683 for #5642. The Rosetta split there covers only VEX scalar compares, because legacy compares were thought not to reach the matcher intact. They do: InstructionScanner passes every instruction with its decoded length, and X64InstructionDecoder decodes F3/F2 0F C2 /r ib with the right length and RIP offset (checked against iced-x86 in #5642's follow-up audit). #5642 measured the same lost load after the legacy forms.

DecodeScalarCompare now also recognises F3 or F2, segment overrides and an optional REX before 0F C2 /r ib, with the same rule as the VEX forms: predicates 0 to 2 stay inline, every other predicate moves out of line unchanged, with a RIP-relative source relocated. A 66, 67 or LOCK prefix, or both F2 and F3, keeps the instruction inline as before.

This branch contains #5683's commit; only the last commit is new. I will rebase it onto main once #5683 is merged.

Tested

Linux, relinker-only build on #5683: ctest passes 158/158 with --timeout 30. New amd64_only_converter cases cover the legacy register, REX, segment-prefixed memory and RIP-relative forms, the predicates that stay inline, the prefixes that are rejected, the fmax shape through the converter and the Intel target; six parser faults each fail them. A differential run of DecodeScalarCompare against iced-x86 over 15,264 valid legacy and VEX encodings (21 prefix sets, 7 REX values, 9 ModRM forms, 8 predicates) finds no case where it moves anything but a hazard CMPSS/CMPSD or misses one, and all 704 RIP-relative relocation targets match. Not run under Rosetta.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: fix(relinker): isolate Rosetta scalar compare hazards #5683
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

LeandroSSB and others added 2 commits October 14, 2026 06:19
The Rosetta split covered only VEX scalar compares. boykopovar#5642 measured the
same lost load after legacy CMPSS and CMPSD, and the decoder already
hands them to the matcher with the right length and RIP offset.

Recognise F3 or F2, segment overrides and an optional REX before
0F C2 /r ib, with the same predicate rule as the VEX forms. A 66, 67 or
LOCK prefix, or both F2 and F3, keeps the instruction inline as before.
@AlexOfTheForest AlexOfTheForest changed the title Fix/rosetta legacy compare fix(relinker): move legacy CMPSS and CMPSD out of line for Rosetta Oct 14, 2026
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 6111 core/relinker/codegen/tests/Amd64OnlyConverterTests.cpp

@jotitadev

Copy link
Copy Markdown
Contributor

Measured under Rosetta 2 on an M2 Max, macOS 27.2 (26B5091g).

Relinker-only build of this branch (ba4b2db23)

AppleClang, Ninja, Release: 149/149 tests pass with --timeout 30.

Legacy compares under Rosetta

Each case runs legacy cmpss/cmpsd $imm, then a 128-bit load into the compare destination, then a scalar compare of that register. I tested predicates 0–7, each with a true and a false result.

Load after the compare CMPSS failures CMPSD failures
SSE movaps / movapd 0/16 0/16
VEX vmovaps / vmovapd 4/16 0/16
  • The four failures are predicates 3, 5 and 6, all with a false result (zero mask).
  • Predicates 0–2 never fail, which matches the rule of keeping them inline.
  • Only a VEX load after the compare triggers the hazard. A legacy compare followed by an SSE load is correct here.
  • I did not see CMPSD fail in this pattern. Moving it out of line is still harmless.

Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.

@011010

011010 commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Verified on macOS 27.0.1 (26A434), Apple M4, x86-64 under Rosetta 2, Apple clang 21: the relinker-only build passes ctest 149/149 with this change applied to current main.

@JoaoPauloCMarra

Copy link
Copy Markdown

ran the relinker output end to end under Rosetta, since the PR says it wasnt run there
M4 Pro, macOS 27.0.1 (26A434), Apple clang 21, PR head ba4b2db, relinker-only build
ctest 149/149 on arm64 and on an x86_64 build under Rosetta (--timeout 30)

same synthetic check as on #5683: 40 sites, each a compare, a vmovaps load and a vucomiss, relinked and then called from an x86_64 harness under Rosetta
legacy cmpss with --to-rosetta: predicates 3-7 move out of line and 0/8 lose the load. on main and on #5683 alone, 3/8 lose it (predicates 3, 4 and 6)
vcmpss stays fixed, 0/32
without --to-rosetta: 15/32 vcmpss and 3/8 cmpss still lose the load, so the repro holds on this branch too

AI-assisted (Claude Code), I ran all of the above on my machine

@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the prefix/REX/segment decoding in DecodeScalarCompare (F3/F2 exclusivity, bounds, RIP-relative relocation), the new amd64_only_converter cases (they fail without the change) and the relinker ctest on the PR merged onto main (134/134). Rosetta runs by @jotitadev, @011010 and @JoaoPauloCMarra confirm legacy cmpss with predicates 3-7 no longer loses the load. Merging.

@adriadam10
adriadam10 merged commit 15a60e1 into boykopovar:main Oct 15, 2026
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants