Skip to content

ZJIT: Only load non-vreg opnds#16486

Merged
tenderlove merged 1 commit into
ruby:masterfrom
tenderlove:load-non-vreg
Mar 23, 2026
Merged

ZJIT: Only load non-vreg opnds#16486
tenderlove merged 1 commit into
ruby:masterfrom
tenderlove:load-non-vreg

Conversation

@tenderlove

Copy link
Copy Markdown
Member

When we call asm.load, many times we're passing in a VReg, and that causes extra loads when we lower to machine code. I'd like to only emit a load in the case that the operand isn't a VReg.

For example this code:

class Foo
  def initialize
    @foo = 123
  end

  def foo
    @foo
  end
end

foo = Foo.new
5.times { foo.foo }

Before this patch, the machine code for LoadField looks like this:

  # Insn: v18 LoadField v17, :_shape_id@0x4
  # Load field id=_shape_id offset=4
  0x121308320: mov x1, x0
  0x121308324: ldur w1, [x1, #4]

Now it looks like this:

  # Insn: v18 LoadField v17, :_shape_id@0x4
  # Load field id=_shape_id offset=4
  0x12339c320: ldur w1, [x0, #4]

We were able to eliminate a reg-reg copy.

Comment thread zjit/src/codegen.rs Outdated
Opnd::mem(64, CFP, RUBY_OFFSET_CFP_SELF)
}

fn load_me_maybe(asm: &mut Assembler, recv: Opnd) -> Opnd {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is a good idea. As long as it's MemBase-compatible, nothing breaks, and it seems more efficient for today's backend.

Let's make Assembler the self so that we can call it with asm.xxx though. It should be the default thing we'll use for any MemBase, so it should have a good-looking API.

Maybe just call it asm.load_mem? It would still need to load imm/uimm/value just in case, but the use of it is going to be mostly mem/reg/vreg, so it would mostly do "load mem".

Comment thread zjit/src/codegen.rs Outdated

fn load_me_maybe(asm: &mut Assembler, recv: Opnd) -> Opnd {
match recv {
Opnd::VReg { .. } => recv,

@k0kubun k0kubun Mar 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I understand correctly, we could also do:

Suggested change
Opnd::VReg { .. } => recv,
Opnd::VReg { .. } | Opnd::Reg(_) => recv,

We might not actually hit that, but I'd feel more comfortable if we excluded the possibility of the unneeded load.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ya, makes sense. I was undecided about whether to add Reg, but thinking about it more, it makes sense.

@tenderlove tenderlove marked this pull request as ready for review March 23, 2026 19:28
@tenderlove tenderlove requested a review from k0kubun March 23, 2026 19:28
@matzbot matzbot requested a review from a team March 23, 2026 19:28
When we call `asm.load`, many times we're passing in a VReg, and that
causes extra loads when we lower to machine code.  I'd like to only emit
a load in the case that the operand _isn't_ a VReg.

For example this code:

```ruby
class Foo
  def initialize
    @foo = 123
  end

  def foo
    @foo
  end
end

foo = Foo.new
5.times { foo.foo }
```

Before this patch, the machine code for `LoadField` looks like this:

```
  # Insn: v18 LoadField v17, :_shape_id@0x4
  # Load field id=_shape_id offset=4
  0x121308320: mov x1, x0
  0x121308324: ldur w1, [x1, #4]
```

Now it looks like this:

```
  # Insn: v18 LoadField v17, :_shape_id@0x4
  # Load field id=_shape_id offset=4
  0x12339c320: ldur w1, [x0, #4]
```

We were able to eliminate a reg-reg copy.

@k0kubun k0kubun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@tenderlove tenderlove merged commit 7444096 into ruby:master Mar 23, 2026
93 checks passed
@tenderlove tenderlove deleted the load-non-vreg branch March 23, 2026 22:09
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.

3 participants