Skip to content
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.

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

YJIT: Use starting context for status === CantCompile #7583

Merged

Conversation

jimmyhmiller
Copy link
Contributor

No description provided.

@matzbot matzbot requested a review from a team March 23, 2023 16:32
@jimmyhmiller
Copy link
Contributor Author

I looked into the failure. It is unrelated to my change. I can actually recreate it locally on master. I will make an issue about it.

@maximecb maximecb merged commit 8286544 into ruby:master Mar 23, 2023
96 of 98 checks passed
@maximecb
Copy link
Contributor

Thanks for fixing this and adding new tests.

@k0kubun
Copy link
Member

k0kubun commented Mar 28, 2023

When we do stack temp register allocation, just looking at a stack operand (even without push/pop) can result in a register spill, i.e. a destructive ctx change. So I'm planning to remove all starting_ctx patterns, including this PR's change.

I'll work on it myself tomorrow, but if you already have insight as to which place should be fixed (is it just the ctx.stack_pop(1) for block arg?), your input would be appreciated.

edit: I changed my mind. The following idea seems easier to maintain.

@k0kubun
Copy link
Member

k0kubun commented Mar 28, 2023

Another idea: If we store asm.insns.len() before compiling each YARV insn, when CantCompile is returned, maybe we could rewind asm to the point before the code for the cant-compile YARV insn is generated. Then we could safely use starting_ctx even with stack temp register allocation.

@maximecb
Copy link
Contributor

Another idea: If we store asm.insns.len() before compiling each YARV insn, when CantCompile is returned, maybe we could rewind asm to the point before the code for the cant-compile YARV insn is generated. Then we could safely use starting_ctx even with stack temp register allocation.

I'm a bit confused.

Normally, we return CantCompile before any instructions are generated? Is this not the case here?

@k0kubun
Copy link
Member

k0kubun commented Mar 28, 2023

Normally, we return CantCompile before any instructions are generated?

This doesn't seem like the case for method calls. invokesuper and invokeblock generating guards before calling gen_send_iseq (which may return CantCompile) are fine-ish because it doesn't touch the stack. However, for OPTIMIZED_METHOD_TYPE_SEND (send(:foo, bar)), you inevitably need to generate a guard that looks at the stack first and then go through the normal method call sequence (which may return CantCompile) again. It doesn't seem trivial to implement the assumption without rewinding an Assembler.

@k0kubun
Copy link
Member

k0kubun commented Mar 28, 2023

I can still look at simply not using starting_ctx for CantCompile though. It's probably too late to avoid generating code before CantCompile, but avoiding stack_pop before CantCompile might be easier than that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants