gh-130887: remove trailing jump in AArch64 JIT stencils#131042
gh-130887: remove trailing jump in AArch64 JIT stencils#131042brandtbucher merged 3 commits intopython:mainfrom
Conversation
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.
diegorusso
requested review from
brandtbucher and
savannahostrowski
as code owners
brandtbucher
left a comment
brandtbucher
left a comment
There was a problem hiding this comment.
Nice, 1.4% faster Linux! No change for macOS, unfortunately.
A couple of comments on the approach:
Tools/jit/_stencils.py
Outdated
| 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) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Have a look. I think I have addressed all your comments.
Tools/jit/_stencils.py
Outdated
| if self.body[offset:] == jump and offset % alignment == 0: | ||
| if self.body[offset:] == jump: | ||
| self.body = self.body[:offset] | ||
| self.disassembly = self.disassembly[:-2] |
There was a problem hiding this comment.
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.
|
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 |
2 participants
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.