ZJIT: Fix ObjToString rewrite
#14196
Conversation
| // Need to replicate what CALL_SIMPLE_METHOD does | ||
| asm_comment!(asm, "side-exit if rb_vm_objtostring returns Qundef"); | ||
| asm.cmp(ret, Qundef.into()); | ||
| asm.je(side_exit(jit, state, ObjToStringFallback)?); |
There was a problem hiding this comment.
I'm not sure how to implement the conditional "call to_s on val when we got Qundef" without generating a lot of assembly (e.g. calling gen_send_without_block and je to it). So I decided to side exit in that case for now. This will at least fix the bug we're seeing
Thoughts?
There was a problem hiding this comment.
You can profile the receiver object, guard on the type (and side-exit), and generate only what's supposed to be executed. This is what YJIT does.
So I decided to side exit in that case for now.
I also agree it's fine for now, but if you're interested in fixing it in this PR, I would suggest:
Profile the receiver type. Generate one of the following HIRs:
- For T_STRING:
GuardType(String) - (Optional: For T_SYMBOL/T_MODULE/T_CLASS/T_NIL/T_TRUE/T_FALSE/T_FIXNUM:
GuardType(type)→ObjToStringthat side-exits on Qundef like the current PR) - For others:
GuardTypeNot(String)(new HIR) →SendWithoutBlock
Behavior-wise, (1) + (3) should fix any issues. (2) is more of an optional optimization that you can leverage your current patch for. GuardTypeNot(String) shouldn't be too hard to implement if you handle only T_STRING (and bail out otherwise) for now.
There was a problem hiding this comment.
I'm not a fan of hiding a send in a non-send HIR instruction either due to optimizer visibility etc. One way around this is to have insn::ObjToString only call rb_vm_objtostring() and build the if-else logic into HIR during parsing. This way we have a regular send in the else BB, and the optimizer should still be able to handle folding away string input situations.
To go faster, see Kokubun's suggestion on how to speculate.
It still sounds quite involved to me though. I think side exiting for now is fine and we can make an issue and optimize this later.
There was a problem hiding this comment.
Thank you for your suggestions. I created Shopify#701 as a follow up.
For now I'll keep side exit so we can avoid the incorrect behaviour.
This comment has been minimized.
This comment has been minimized.
|
I updated the comment at #14196 (comment) and merged my suggestions there. |
|
If it's too complicated to figure out the right HIR for it now, I guess it's fine to start with this and work on it in a separate PR. Could you file another issue to optimize |
| @@ -439,6 +439,24 @@ fn gen_get_ep(asm: &mut Assembler, level: u32) -> Opnd { | |||
| ep_opnd | |||
| } | |||
|
|
|||
| fn gen_objtostring(jit: &mut JITState, asm: &mut Assembler, val: Opnd, cd: *const rb_call_data, state: &FrameState) -> Option<Opnd> { | |||
| gen_prepare_non_leaf_call(jit, asm, state)?; | |||
There was a problem hiding this comment.
I couldn't figure out which part of rb_vm_objtostring calls a method. Could you point out which part of it is not leaf?
There was a problem hiding this comment.
When recv.singleton_class? rb_mod_to_s() calls can call inspect on the attached object. Diabolical.
There was a problem hiding this comment.
Oh I realized there's rb_inspect in the non-rb_any_to_s branch, which does rb_funcall. So yeah, rb_vm_objtostring is not leaf.
It doesn't matter at this point, but I didn't get what's not leaf about rb_any_to_s. Does rb_sprintf("#<%"PRIsVALUE":%p>", cname, (void*)obj) dispatch a method through PRIsVALUE? (I don't know which code formats "%"PRIsVALUE)
There was a problem hiding this comment.
Yes, in general PRIsVALUE can call to_s by going through PRI_EXTRA_MARK and ruby__sfvextra(). Only for non-T_STRINGs does ruby__sfvextra() make a call, though, and it looks like cname is always a string here. Though there is also an encoding check in there so the print might rise.
Currently, the rewrite for `ObjToString` always replaces it with a `SendWithoutBlock(to_s)` instruction when the receiver is not a string literal. This is incorrect because it calls `to_s` on the receiver even if it's already a string. This change fixes it by: - Avoiding the `SendWithoutBlock(to_s)` rewrite - Implement codegen for `ObjToString`
9492f5f to
bed681e
Compare
Currently, the rewrite for
ObjToStringalways replaces it with aSendWithoutBlock(to_s)instruction when the receiver is not a string literal. This is incorrect because it callsto_son the receiver even if it's already a string.This change fixes it by:
SendWithoutBlock(to_s)rewriteObjToString