gh-118926: Spill deferred references to stack in cases generator #122748
Conversation
This automatically spills the results from `_PyStackRef_FromPyObjectNew` to the in-memory stack so that the deferred references are visible to the GC before we make any possibly escaping call. Co-authored-by: Ken Jin <kenjin@python.org>
|
@markshannon, this is the spilling logic split out from: I've updated it to handle the changes to the stack from #122286. This required tracking output local variables in the stack before emitting tokens for a uop. I've also moved the analysis to analyzer.py. |
There was a problem hiding this comment.
The stack shouldn't have "outputs" (or inputs) as it should model the state of the stack only.
The outputs (or inputs) are properties of the uops and instructions.
That can be fixed by passing the output variables to flush_single_var instead of caching them on the stack.
Note:
I think the correct long term approach to handling stack spilling is to lazily spill when code escapes. But that involves considerably deeper analysis.
Although this PR doesn't do that, it doesn't prevent us from doing in future. So I am happy to merge it once the above issue is fixed.
Tools/cases_generator/analyzer.py
Outdated
| if tkn.kind != "IDENTIFIER" or tkn.text != "PyStackRef_FromPyObjectNew": | ||
| continue | ||
|
|
||
| if node.block.tokens[idx-1].kind != "EQUALS": |
There was a problem hiding this comment.
I think this is OK, as the first token should be the opening brace. Could you add a comment as to why this is safe.
There was a problem hiding this comment.
I think that's right, but I'll add condition to ensure that idx != 0 to make it explicit.
Tools/cases_generator/stack.py
Outdated
| @@ -169,6 +169,7 @@ def __init__(self) -> None: | |||
| self.top_offset = StackOffset.empty() | |||
| self.base_offset = StackOffset.empty() | |||
| self.variables: list[Local] = [] | |||
| self.outputs: list[Local] = [] | |||
There was a problem hiding this comment.
The stack shouldn't have "outputs" (or inputs) as it should model the state of the stack.
The outputs (or inputs) are properties of the uops and instructions.
Tools/cases_generator/stack.py
Outdated
| @@ -290,6 +302,22 @@ def flush(self, out: CWriter, cast_type: str = "uintptr_t", extract_bits: bool = | |||
| self.base_offset.clear() | |||
| self.top_offset.clear() | |||
|
|
|||
| def flush_single_var(self, out: CWriter, var_name: str, cast_type: str = "uintptr_t", extract_bits: bool = False) -> None: | |||
There was a problem hiding this comment.
Rather than caching outputs on the stack, can you pass outputs as a list[StackItem] argument?
There was a problem hiding this comment.
I added outputs as a list[StackItem]
| @@ -112,7 +112,14 @@ def write_uop( | |||
| out.emit(code) | |||
| if local.defined: | |||
| locals[local.name] = local | |||
| out.emit(stack.define_output_arrays(prototype.stack.outputs)) | |||
| outputs: list[Local] = [] | |||
There was a problem hiding this comment.
The optimizer generator doesn't need to track the state of locals, nor is the GC involved, so shouldn't change.
There was a problem hiding this comment.
I've reverted the changes to optimizer_generator.py
|
When you're done making the requested changes, leave the comment: And if you don't make the requested changes, you will be poked with soft cushions! |
|
@markshannon, would you please take another look at it. I have made the requested changes; please review again |
|
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
colesbury
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit ae1c018 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
python#122748) This automatically spills the results from `_PyStackRef_FromPyObjectNew` to the in-memory stack so that the deferred references are visible to the GC before we make any possibly escaping call. Co-authored-by: Ken Jin <kenjin@python.org>
This automatically spills the results from
_PyStackRef_FromPyObjectNewto the in-memory stack so that the deferred references are visible to the GC before we make any possibly escaping call.