Skip to content

ZJIT: Panic unimplemented for OOB basic block args#13533

Merged
k0kubun merged 1 commit intoruby:masterfrom
havenwood:zjit-register-expansion
Jun 5, 2025
Merged

ZJIT: Panic unimplemented for OOB basic block args#13533
k0kubun merged 1 commit intoruby:masterfrom
havenwood:zjit-register-expansion

Conversation

@havenwood
Copy link
Contributor

  • Go from 8 to 10 ALLOC_REGS registers for ARM64 and x86_64
  • Panic about unimplemented register spilling when block args are out of bounds

Testing ZJIT I noticed an index out of bounds panic with:

ruby --zjit -e "pp 0"

Pending the implementation of register spilling, I propose:

  1. Improve the panic message when there aren't enough registers for block arguments
  2. Increase the number of basic block registers from 8 to 10 to cover many more cases in the interim.

@matzbot matzbot requested a review from a team June 5, 2025 21:25
R10_REG,
RAX_REG,
R14_REG,
R15_REG,
Copy link
Member

Choose a reason for hiding this comment

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

r14 and r15 are callee-saved registers. The register allocator currently doesn't support callee-saved registers, so they're deliberately not added to the list at the moment. This branch could cause an undefined behavior depending on a C function called in JIT code. We'd need to preserve them when entering/leaving JIT code, but that's also overhead we need to carefully handle.

So our path forward is probably to support register spill first, before expanding the register pool to callee-saved registers.

use super::*;

#[test]
#[should_panic(expected = "register spilling not yet implemented, too many basic block arguments (11/10)")]
Copy link
Member

Choose a reason for hiding this comment

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

The panic (by unimplemented!) itself works as an inline test, so I don't think you should test the test. It's also tedious to update the "11/10" part whenever you touch the ALLOC_REGS constant.

@havenwood havenwood force-pushed the zjit-register-expansion branch from f5d682f to 3a5e260 Compare June 5, 2025 23:28
@havenwood havenwood changed the title ZJIT: Expand registers for basic block arguments ZJIT: Panic unimplemented for OOB basic block args Jun 5, 2025
@havenwood
Copy link
Contributor Author

Thank you for explaining why I was in error, @k0kubun. I very much appreciate your thoughtful response. I've updated the PR accordingly, removing the new registers and unnecessary test.

I updated this PR to only include the unimplemented panic message.

Copy link
Member

@k0kubun k0kubun left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@k0kubun k0kubun merged commit 43472a3 into ruby:master Jun 5, 2025
48 of 65 checks passed
@havenwood havenwood deleted the zjit-register-expansion branch June 5, 2025 23:39
sms021019 pushed a commit to sms021019/ruby that referenced this pull request Jun 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants