Skip to content

ZJIT: Use Mem.num_bits in Mem split#15177

Merged
tekknolagi merged 3 commits intoruby:masterfrom
tekknolagi:mb-fix-mem-split
Nov 14, 2025
Merged

ZJIT: Use Mem.num_bits in Mem split#15177
tekknolagi merged 3 commits intoruby:masterfrom
tekknolagi:mb-fix-mem-split

Conversation

@tekknolagi
Copy link
Contributor

@tekknolagi tekknolagi commented Nov 13, 2025

No description provided.

@tekknolagi
Copy link
Contributor Author

tekknolagi commented Nov 13, 2025

Should fix the

write(2, "ruby: ZJIT has panicked. More info to follow...\n", 48) = 48
write(2, "\nthread '<unnamed>' panicked at zjit/src/backend/lir.rs:160:17:\nassertion failed: num_bits <= out_num_bits\n", 107) = 107

based on

#25 0x0000aaaaaae8fb14 in zjit::backend::lir::Opnd::mem (num_bits=64, base=..., disp=0) at zjit/src/backend/lir.rs:160
#26 zjit::backend::arm64::{impl#3}::arm64_split::split_memory_address (asm=<optimized out>,
    opnd=<error reading variable: Cannot access memory at address 0x0>) at zjit/src/backend/arm64/mod.rs:260
#27 zjit::backend::arm64::{impl#3}::arm64_split::split_load_operand (asm=<optimized out>, opnd=...) at zjit/src/backend/arm64/mod.rs:273

but I can't tell how to test this

@tekknolagi tekknolagi marked this pull request as ready for review November 13, 2025 18:49
@matzbot matzbot requested a review from a team November 13, 2025 18:49
@XrXr
Copy link
Member

XrXr commented Nov 13, 2025

but I can't tell how to test this

try asm.mov(Opnd::mem(32, X0, 0), Opnd::mem(32, X1, 0))? Boilerplate at the end of this file.

@k0kubun
Copy link
Member

k0kubun commented Nov 13, 2025 •
edited
Loading

try asm.mov(Opnd::mem(32, X0, 0), Opnd::mem(32, X1, 0))

It's for a large displacement, so I think you need to use something like Opnd::mem(32, X0, 512) (or anything that trips mem_disp_fits_bits on L256) in one of these operands.

For example:

    #[test]
    fn test_split_large_disp() {
        let (mut asm, mut cb) = setup_asm();

        asm.mov(C_RET_OPND, Opnd::mem(32, C_RET_OPND, 512));
        asm.compile(&mut cb).unwrap();

        assert_disasm_snapshot!(cb.disasm(), @r"
        ...
        ");
        assert_snapshot!(cb.hexdump(), @"...");
    }

@tekknolagi
Copy link
Contributor Author

tekknolagi commented Nov 13, 2025 

Failed to compile LIR at insn_idx=0:
=>  w0 = Lea Mem32[x0 + 0x200]
    w0 = Load Mem32[x0]


thread 'backend::arm64::tests::test_split_mem_mem_with_large_displacement' (20216797) panicked at zjit/src/asm/arm64/mod.rs:67:13:
rd and rn must be of the same size.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused non-unwinding panic. aborting.

We're getting somewhere........

@tekknolagi
Copy link
Contributor Author

tekknolagi commented Nov 13, 2025

This is with

        let vreg = asm.load(Opnd::mem(32, C_RET_OPND, 512));
        asm.compile(&mut cb);

@tekknolagi
Copy link
Contributor Author

tekknolagi commented Nov 13, 2025

I have a growing sense of unease that our scratch reg splitting might not use the right register sizes in all cases

@tekknolagi
Copy link
Contributor Author

tekknolagi commented Nov 13, 2025

I think we got it!

XrXr
XrXr approved these changes Nov 13, 2025
View reviewed changes
Copy link
Member

@XrXr XrXr left a comment

Choose a reason for hiding this comment

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

nit: s/512/0x200/ so they look the same in rust source and in the snapshot

@tekknolagi tekknolagi enabled auto-merge (squash) November 14, 2025 00:01
@launchable-app
Copy link

launchable-app bot commented Nov 14, 2025

❌ 1/66963 Tests Failed

test/io/wait/test_io_wait.rb#test_wait 
Failure:
TestIOWait#test_wait [D:/a/ruby/ruby/src/test/io/wait/test_io_wait.rb:27]:
<#<Socket:fd 5>> expected but was
<nil>.

[-> View Test suite health in main branch]

@tekknolagi tekknolagi merged commit c92a44e into ruby:master Nov 14, 2025
92 of 94 checks passed
@tekknolagi tekknolagi deleted the mb-fix-mem-split branch November 14, 2025 03:02
@k0kubun
Copy link
Member

k0kubun commented Nov 14, 2025

I have a growing sense of unease that our scratch reg splitting might not use the right register sizes in all cases

"our scratch reg splitting" is in arm64_scratch_split, not this arm64_split btw. It's split_large_disp in arm64_scratch_split, and we already addressed the problem in that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@XrXr XrXr XrXr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tekknolagi @XrXr @k0kubun