Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-130887: remove trailing jump in AArch64 JIT stencils #131042

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

diegorusso
Copy link
Contributor

@diegorusso diegorusso commented Mar 10, 2025

In order to keep the alignment of the code body, the removed jump and the 0 padding at the end of AArch64 JIT stencils have been replaced with nop instructions.

In order to keep the alignment of the code body, the removed
jump and the 0 padding at the end of AArch64 JIT stencils
have been replaced with nop instructions.
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, 1.4% faster Linux! No change for macOS, unfortunately.

A couple of comments on the approach:

Comment on lines +212 to +218
def add_nop(self, alignment: int) -> None:
"""Add a NOP if the offset is not aligned."""
offset = len(self.body)
nop = b"\x1f\x20\x03\xD5"
if offset % alignment:
self.disassembly.append(f"{offset:x}: d503201f\t\t nop")
self.body.extend(nop)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that this is ARM-specific, and only really happens to work because of the current alignment values we chose.

I wonder how messy it would be to make nop an attribute of _Target, and pass it into process_relocations, which passes it here (sort of like alignment).

I think that we should probably also make this so that it can add several nops if the alignment is larger (such as 16), and fail if this isn't possible (when adding nops overshoots the intended alignment).

self.body = self.body[:offset]
self.disassembly = self.disassembly[:-2]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, I'd leave the disassembly alone. I get what you're doing here, but I'd rather leave it as what the preprocessed stencil used to look like and not try to do anything too clever.

@bedevere-app
Copy link

bedevere-app bot commented Mar 18, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

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.

2 participants