Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-131798: JIT: SplitCALL_TYPE_1 into several uops#132419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Python/optimizer_bytecodes.c Outdated
| sym_set_null(null); | ||
| } | ||
| op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) { |
There was a problem hiding this comment.
Choose a reason for 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 getSyntaxError: Duplicate name unused, thoughbytecodes.c allows it 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Weird. Maybeunused isn't a meaningful value here like it is inbytecodes.c (maybe@markshannon knows)?
There was a problem hiding this comment.
Choose a reason for 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok!
There was a problem hiding this comment.
Choose a reason for 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
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Python/optimizer_bytecodes.c Outdated
| sym_set_null(null); | ||
| } | ||
| op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Weird. Maybeunused isn't a meaningful value here like it is inbytecodes.c (maybe@markshannon knows)?
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 |
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.
Just to update the status: I have made the requested changes; please review again |
Thanks for making the requested changes! @brandtbucher: please review the changes made to this pull request. |
There was a problem hiding this comment.
Choose a reason for 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.
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: InfileincludedfromPython/ceval.c:1121:Python/executor_cases.c.h:Infunction ‘_PyEval_EvalFrameDefault’:Python/executor_cases.c.h:5142:25:warning:variable ‘callable’setbutnotused [-Wunused-but-set-variable]5142 |_PyStackRefcallable; | ^~~~~~~~Python/executor_cases.c.h:5141:25:warning:variable ‘null’setbutnotused [-Wunused-but-set-variable]5141 |_PyStackRefnull; | ^~~~ |
Yeah, we should fix those. Can you rename them to |
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.cindex 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) = |
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. |
brandtbucher left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for 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 inmain andmake regen-cases to fix the conflicts).
Python/optimizer_bytecodes.c Outdated
| op(_CALL_TYPE_1, (callable, null, arg -- res)) { | ||
| (void)callable; | ||
| (void)null; |
There was a problem hiding this comment.
Choose a reason for 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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm that doesn't help silence the warnings inexecutor_cases.c.h :/ only thing that I found that works is(void)foo like in0925179. Is there a better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nope, that's fine!
a6a3dbb intopython:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This mostly works now, though the there are now two unused stackref variables in
executor_cases.c.h(nullandcallable) and I'm not sure how to convince the cases generator to remove them. I tried:DEAD(x)in the guard uops - does not work because the stack variable also appears in the output of the uop. I also can't easily pop them off the stack since neithernullorcallableis TOS.DEAD(x)from_CALL_TYPE_1: not possible, apparently this is needed before I can setres.I'm out of ideas so if anyone knows how to fix it, I'd love to know :)