Skip to content

ZJIT: Codegen for NewHash #14059

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

ZJIT: Codegen for NewHash #14059

wants to merge 16 commits into from
+168 −3

Conversation

corona10
Copy link

This is my first contribution to ZJIT. I started by handling NewHash, and also noticed that more complex cases like HashDup and Send should be handled as well. These might be optimizable into other opcodes.

I'm sure my mentor @tekknolagi will take a look :)

@matzbot matzbot requested a review from a team July 31, 2025 03:51
tekknolagi
tekknolagi requested changes Jul 31, 2025
View reviewed changes
Copy link
Contributor

@tekknolagi tekknolagi left a comment

Choose a reason for hiding this comment

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

Thank you for the very fast PR! On the right track -- needs some more work.

@tekknolagi
Copy link
Contributor

Also I wonder if this failure is related:

  make[1]: *** [yes-test-all] Error 1
    1) Failure:
  make: *** [zjit-test-all] Error 2
  TestRubyOptimization#test_side_effect_in_popped_splat [/Users/runner/work/ruby/ruby/src/test/ruby/test_optimization.rb:984]:
  Expected "[ruby-core:84340] [Bug #14201]" to be nil.

@tekknolagi tekknolagi linked an issue Jul 31, 2025 that may be closed by this pull request
ZJIT: Codegen for NewHash Shopify/ruby#662
Open
@corona10 corona10 changed the title ZJIT: Codegen for NewHash [WIP] ZJIT: Codegen for NewHash Jul 31, 2025
@corona10
Copy link
Author

corona10 commented Aug 1, 2025

Also I wonder if this failure is related:

Not sure.. I can not reproduce at my mahcine.

@corona10
Copy link
Author

corona10 commented Aug 1, 2025

yeah it's related.. I need to understand intended behavior.

@launchable-app Launchable app
Copy link

launchable-app bot commented Aug 3, 2025 •
edited
Loading

❌ Tests Failed

✖️no tests failed ✔️62296 tests passed(2 flakes)

@corona10 corona10 changed the title [WIP] ZJIT: Codegen for NewHash ZJIT: Codegen for NewHash Aug 3, 2025
@corona10 corona10 requested a review from tekknolagi August 3, 2025 13:31
@corona10
Copy link
Author

corona10 commented Aug 3, 2025

@tekknolagi Thank for the review explain many things :) I've updated the PR!

k0kubun
k0kubun reviewed Aug 4, 2025
View reviewed changes
@tekknolagi
Copy link
Contributor

Except for @k0kubun 's feedback I think this looks great! Bulk insert is a good idea.

(Kokubun, do you think the frame setup bit is ok?)

@k0kubun
Copy link
Member

k0kubun commented Aug 4, 2025 •
edited
Loading

Kokubun, do you think the frame setup bit is ok?

Yeah, it's a non-leaf instruction, so it makes sense that we set PC/SP and spill locals/stack slots (for now). Filed #14100 to define a helper for sharing the logic.

When there's no element to insert, it's probably leaf (while it does trigger GC). I think we can skip the SP update and spills in that case. We still have to set PC for allocation tracing.

k0kubun
k0kubun reviewed Aug 4, 2025
View reviewed changes
@tekknolagi
Copy link
Contributor

There are some test failures that are not immediately clear what's going on. I can try and take a look tomorrow or the day after. Frustrating :/ Probably something small

k0kubun
k0kubun reviewed Aug 6, 2025
View reviewed changes
k0kubun
k0kubun reviewed Aug 6, 2025
View reviewed changes
k0kubun
k0kubun reviewed Aug 6, 2025
View reviewed changes
zjit/src/codegen.rs Outdated
}

for (i, pair) in pairs.iter().enumerate() {
let stack_opnd = Opnd::mem(64, SP, i as i32 * SIZEOF_VALUE_I32);
Copy link
Member

@k0kubun k0kubun Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This seems unsafe. rb_hash_bulk_insert calls hash functions, which may not be leaf (hence you're calling gen_prepare_non_leaf_call).

Your code here is writing above the VM stack stop, so if anything is pushed to the VM stack, the content of the array in the argument gets clobbered. Because it's not leaf, it could call another method, and at that point things will be pushed to the VM stack.

But you can't bump the cfp->sp to protect them because it would miss the point of gen_prepare_non_leaf_call (part of the dumped stack should be a valid array usable for rb_hash_bulk_insert currently, but we'd want to optimize it away later anyway), which makes it safe for the interpreter to take over the stack as is.

So I think we need to use the C stack here. Bump NATIVE_STACK_PTR, use memory operands based off of NATIVE_BASE_PTR (because NATIVE_STACK_PTR can be moved by the backend), and restore NATIVE_STACK_PTR.

Copy link
Member

Choose a reason for hiding this comment

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

Well, I remembered we never update the SP register, so I guess I misread it a bit. But then it writes to random locations in the VM stack that already have spilled stack slots, so it can still corrupt the state for the interpreter in the wrong way. As suggested above, we should write to the C stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to pair on this this week or next @corona10 ! Sorry it's been a bit of a rollercoaster

@corona10 @k0kubun
Apply suggestions from code review
531ad3c 
Co-authored-by: Takashi Kokubun <takashikkbn@gmail.com>
@k0kubun k0kubun mentioned this pull request Aug 8, 2025
Merged
@tekknolagi
Copy link
Contributor

Another thing we can do to chunk the work is to add a new PR that just handles NewHash with no operands. That should be a) much easier to compile and b) still handle a huge chunk of the "failed to compile" cases which are just allocating empty hashes.

@corona10
Copy link
Author

Another thing we can do to chunk the work is to add a new PR that just handles NewHash with no operands. That should be a) much easier to compile and b) still handle a huge chunk of the "failed to compile" cases which are just allocating empty hashes.

Yeah, let's start with simple stuff first. (I was also pretty busy with other stuff, but happy to start pair with you too :))

@corona10 corona10 requested a review from k0kubun August 13, 2025 16:29
@tekknolagi
Copy link
Contributor

I wonder if you are running into a manifestation of https://bugs.ruby-lang.org/issues/14201 that #14205 fixes

@corona10
Copy link
Author

I wonder if you are running into a manifestation of https://bugs.ruby-lang.org/issues/14201 that #14205 fixes

Maybe it is worth rebasing PR once you merge it.

@tekknolagi
Copy link
Contributor

Feel free to also cherry-pick

k0kubun
k0kubun reviewed Aug 13, 2025
View reviewed changes
k0kubun
k0kubun reviewed Aug 13, 2025
View reviewed changes
@k0kubun
Copy link
Member

k0kubun commented Aug 13, 2025 •
edited
Loading

Now that the PR is merged, let's try rebasing from the latest master then? We can't merge a PR with a failing ZJIT test, which is relatively stable these days.

@corona10 corona10 closed this Aug 14, 2025
@corona10 corona10 reopened this Aug 14, 2025
@matzbot matzbot requested a review from a team August 14, 2025 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k0kubun k0kubun k0kubun left review comments

@tekknolagi tekknolagi Awaiting requested review from tekknolagi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ZJIT: Codegen for NewHash
3 participants
@corona10 @tekknolagi @k0kubun