Repository navigation
GH-130415: Improve the JIT's unneeded uop removal pass - #132333
brandtbucher merged 5 commits into
Conversation
Sorry, something went wrong.
Fidget-Spinner
left a comment
There was a problem hiding this comment.
I think this should be fine? optimize_pop_top_again should reach a fixpoint (there are only a finite number of last we can change to _NOP).
Sorry, something went wrong.
markshannon
left a comment
There was a problem hiding this comment.
I have a few questions.
The overall design looks good.
Sorry, something went wrong.
| load_attr_bottom = opnames.index("_LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES", call) | ||
| self.assertEqual(opnames[:load_attr_top].count("_GUARD_TYPE_VERSION"), 1) | ||
| self.assertEqual(opnames[call:load_attr_bottom].count("_CHECK_VALIDITY"), 1) | ||
| self.assertEqual(opnames[call:load_attr_bottom].count("_CHECK_VALIDITY"), 2) |
There was a problem hiding this comment.
Why do we have two _CHECK_VALIDITYs now?
Sorry, something went wrong.
There was a problem hiding this comment.
There used to be one validity check, after the setattr call. This adds an additional one after the _POP_TOP following the call, since in theory the function could return an object with a destructor that invalidates the JIT code when it's popped.
Because of the old structure of this optimization pass, we weren't actually treating _POP_TOP as escaping, even though it is (this is handled in the default case, which we fall through to now).
Sorry, something went wrong.
| switch (last->opcode) { | ||
| case _POP_TWO_LOAD_CONST_INLINE_BORROW: | ||
| last->opcode = _POP_TOP; | ||
| goto optimize_pop_top_again; |
There was a problem hiding this comment.
Do we want to optimize again here?
If the previous instruction is _POP_TOP I see no further optimization to apply here.
Sorry, something went wrong.
There was a problem hiding this comment.
Nice catch. We might want to combine smaller ops into larger ones later (for example, turn _POP_TOP; _POP_TOP_LOAD_CONST_INLINE_BORROW into _NOP; _POP_TWO_LOAD_CONST_INLINE_BORROW), which is where I think I was going with this. But that can come later.
Sorry, something went wrong.
| last->opcode = _NOP; | ||
| buffer[pc].opcode = _NOP; | ||
| } | ||
| if (last->opcode == _REPLACE_WITH_TRUE) { |
There was a problem hiding this comment.
Are we not optimizing this any more? I would have thought it would go in the same case as _POP_TOP_LOAD_CONST_INLINE_BORROW.
Sorry, something went wrong.
There was a problem hiding this comment.
Nice catch. Though these should probably just be turned into _POP_TOP_LOAD_CONST_INLINE_BORROW by the optimizer instead.
I'll leave it this way and open up another PR for that. Having the hardcoded constant makes things more difficult, since we'd need to start writing operands instead of operating purely on opcodes.
Sorry, something went wrong.
There was a problem hiding this comment.
Actually, I just added that change as part of this PR.
Sorry, something went wrong.
This improves our ability to remove unused constant and local variable loads during the remove_unneeded_uops pass in the JIT's optimizer.
It now handles all combinations of:
...followed by...
(It also fixes an issue where CHECK_VALIDITY wasn't being added after these latter four instructions, due to how the old switch-case was structured.)
According to the stats, this results in:
@savannahostrowski: We should see even more impact once you start using _POP_TWO_LOAD_CONST_INLINE_BORROW in more places. I'm currently working on turning basically all cached class attribute/method loads into constants too, which should help here as well.