-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-129552: Make condition inside codegen_addop_load_const
a macro
#129553
Conversation
f73717d
to
e878644
Compare
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'd prefer this become an assertion.
I'd prefer removing the < 256 and keep the macro constant as this could be changed in the future. Currently this is 257 so it is indeed redundant but if we remove the < 256, we might want to rather decide whether to use LOAD_SMALL_INT opcode depending on the value of that macro rather than the hardcoded 256. |
Note: I remember I asked Mark about it at some point but forgot where I asked. I think we had assertions indeed but I don't remember whether we used the macro or a hardcoded constant. |
But aren't we also restricted by magnitude of oparg here? We probably don't want to use extended args for this. |
Ah yes.. the oparg. Right. Thanks Irit. That was the reason why we had the hardcoded 256. Nevermind my suggestion then, but we can keep an assertion as Peter suggested. |
Yes, at first I tried to remove 256 check but then realized that extended arg is generated in that case which I suppose is not desired behavior. What assertion and where? I don't quite get. |
In the if-block, just assert that it's also smaller than the |
It is already guarded by |
Assertions are cost-free but if the value of that macro (small ints) changes, we might have some surprises (as we would have an array OOB at some point I think, but only if we make it smaller than 256) Ideally I would prefer to have 256 as a macro to stress that it's for a non-extended oparg, as it could be confused with the max small posint but I think that this limit (256) is generally hardcoded. |
codegen_addop_load_const
codegen_addop_load_const
a macro
So based on #129568 (comment), I am leaving this condition as it is and converting it to a macro to be reused in CFG. cc @iritkatriel |
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Closing this one as it will be done as part of another issue. |
codegen_addop_load_const
a macro #129552