gh-126902: Strength reduce _CHECK_FUNCTION#126856
gh-126902: Strength reduce _CHECK_FUNCTION#126856Fidget-Spinner wants to merge 6 commits intopython:mainfrom
Conversation
|
@markshannon would you prefer this as its own PR, or would you prefer I merge this into the function inlining PR that I will be doing later? |
|
Sorry I realized this is logically incorrect. |
Fidget-Spinner
force-pushed
the
hoist_check_function
branch
from
5fbc47a to
462cf77
Compare
Fidget-Spinner
force-pushed
the
hoist_check_function
branch
from
462cf77 to
115bd0e
Compare
Fidget-Spinner
changed the title
|
I think I've said this before, but please link to an appropriate issue. Why are you doing this? The issue doesn't explain it at all. |
Fidget-Spinner
changed the title
|
@markshannon the issue is right, just too broad. I've created a smaller issue to further explain why we need this for partial evaluation at #126902 |
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
I think it would be better to use the symbol for the function, rather than a reference in the optimizer.
| with self.assertRaises(TypeError): | ||
| {item for item in items} | ||
|
|
||
| def test_global_guard_strength_reduced_if_possible(self): |
There was a problem hiding this comment.
It would be nicer (not necessarily in this PR) if we created tracelets directly, rather than this complex approach.
|
When you're done making the requested changes, leave the comment: And if you don't make the requested changes, you will be put in the comfy chair! |
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
I'd like to replace NULL with sym_new_unknown when f_funcobj is not known.
Otherwise, LGTM.
|
|
||
| tier2 op(_CHECK_FUNCTION_UNMODIFIED, (func_version/2, callable_p/4 --)) { | ||
| assert(PyFunction_Check(callable_p)); | ||
| PyFunctionObject *func = (PyFunctionObject *)callable_p; |
There was a problem hiding this comment.
What guarantee is there that func == frame->f_funcobj?
There was a problem hiding this comment.
It's constant propagated (and frames shouldn't be able to swap out f_funcobj on the fly like that). To be sure I added a runtime assert.
|
All commit authors signed the Contributor License Agreement. |
2 participants
Small PR to strength reduce _CHECK_FUNCTION so it reads from operand instead of reading from the frame struct. This prevents _CHECK_FUNCTION from interfering with the partial evaluation later on. It interferes because once we remove the frame, there is no
frame->func_objto check anymore.An added benefit is that the "burned in" version is also cheaper as it doesn't have to read from the frame struct, rather reading from the instruction itself in the JIT.