gh-118926: Spill deferred references to stack in cases generator#122748
Uh oh!
There was an error while loading. Please reload this page.
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>
colesbury commented Aug 6, 2024
@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. |
markshannon left a comment
markshannon left a comment 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
| iftkn.kind!="IDENTIFIER"ortkn.text!="PyStackRef_FromPyObjectNew": | ||
| continue | ||
| ifnode.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
| 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
| self.base_offset.clear() | ||
| self.top_offset.clear() | ||
| defflush_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]
| iflocal.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! |
colesbury commented Aug 7, 2024
@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. |
markshannon left a comment
markshannon left a comment There was a problem hiding this comment.
Thanks for bearing with me on this one.
colesbury added the 🔨 test-with-refleak-buildbots 
bedevere-bot commented Aug 7, 2024
🤖 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>
3 participants
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.