GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-131798: JIT: Split `CALL_TYPE_1` into several uops by tomasr8 · Pull Request #132419 · python/cpython Conversation
|
| sym_set_null(null); | ||
| } | ||
|
|
||
| op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I am not entirely sure why I cannot do this here:
| op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) { | |
| op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused -- callable, unused, unused)) { |
I get SyntaxError: Duplicate name unused, though bytecodes.c allows it 🤔
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Weird. Maybe unused isn't a meaningful value here like it is in bytecodes.c (maybe @markshannon knows)?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
We can leave it like this. I'll create an issue for Mark to update the cases generator and mention this line.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
ok!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Appears to be fixed now thanks to #132615, I updated the PR
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| sym_set_null(null); | ||
| } | ||
|
|
||
| op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Weird. Maybe unused isn't a meaningful value here like it is in bytecodes.c (maybe @markshannon knows)?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Brandt Bucher <[email protected]>
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Brandt Bucher <[email protected]>
Co-authored-by: Brandt Bucher <[email protected]>
There was an error while loading. Please reload this page.
|
Just to update the status: I have made the requested changes; please review again |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for making the requested changes! @brandtbucher: please review the changes made to this pull request. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Up to you if you want to add the additional test. I'll leave this open for a bit longer just in case.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I added the test you suggested since it was quite easy to do :) Also, should we care about the compiler warnings I mentioned in the PR description? There are now two unused variable warnings: In file included from Python/ceval.c:1121: Python/executor_cases.c.h: In function ‘_PyEval_EvalFrameDefault’: Python/executor_cases.c.h:5142:25: warning: variable ‘callable’ set but not used [-Wunused-but-set-variable] 5142 | _PyStackRef callable; | ^~~~~~~~ Python/executor_cases.c.h:5141:25: warning: variable ‘null’ set but not used [-Wunused-but-set-variable] 5141 | _PyStackRef null; | ^~~~ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yeah, we should fix those. Can you rename them to |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
it's getting a bit late here so maybe I missed something, but I'm getting diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 22bb4097611..129e606bde2 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -3969,15 +3969,14 @@ dummy_func( DEOPT_IF(callable_o != (PyObject *)&PyType_Type); } - op(_CALL_TYPE_1, (callable, null, arg -- res)) { + op(_CALL_TYPE_1, (unused, unused, arg -- res)) { PyObject *arg_o = PyStackRef_AsPyObjectBorrow(arg); assert(oparg == 1); - DEAD(null); - DEAD(callable); STAT_INC(CALL, hit); res = PyStackRef_FromPyObjectNew(Py_TYPE(arg_o)); PyStackRef_CLOSE(arg); + INPUTS_DEAD(); } macro(CALL_TYPE_1) = |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
How about we add (void)null; (void)callable; I've seen it used in some other instructions. Not sure whether it's the best fix but it does get rid of the warnings. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just one more suggestion (then you should merge in main and make regen-cases to fix the conflicts).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| op(_CALL_TYPE_1, (callable, null, arg -- res)) { | ||
| (void)callable; | ||
| (void)null; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This should work!
| op(_CALL_TYPE_1, (callable, null, arg -- res)) { | |
| (void)callable; | |
| (void)null; | |
| op(_CALL_TYPE_1, (unused, unused, arg -- res)) { |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Hmm that doesn't help silence the warnings in executor_cases.c.h :/ only thing that I found that works is (void)foo like in 0925179. Is there a better way?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Nope, that's fine!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.