Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

WolframAlph
Copy link
Contributor

@WolframAlph WolframAlph commented Feb 1, 2025

@WolframAlph WolframAlph marked this pull request as ready for review February 1, 2025 17:02
Copy link
Member

@ZeroIntensity ZeroIntensity left a 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.

@picnixz
Copy link
Member

picnixz commented Feb 1, 2025

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.

cc @markshannon @Eclips4

@picnixz
Copy link
Member

picnixz commented Feb 1, 2025

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.

@iritkatriel
Copy link
Member

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.

cc @markshannon @Eclips4

But aren't we also restricted by magnitude of oparg here? We probably don't want to use extended args for this.

@picnixz
Copy link
Member

picnixz commented Feb 1, 2025

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.

@WolframAlph
Copy link
Contributor Author

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.

@picnixz
Copy link
Member

picnixz commented Feb 1, 2025

In the if-block, just assert that it's also smaller than the _PY_NSMALLPOSINTS macro value (I guess that's what @ZeroIntensity wanted)

@WolframAlph
Copy link
Contributor Author

WolframAlph commented Feb 1, 2025

In the if-block, just assert that it's also smaller than the _PY_NSMALLPOSINTS macro value (I guess that's what @ZeroIntensity wanted)

It is already guarded by val < 256. Isn't it even more redundant?

@picnixz
Copy link
Member

picnixz commented Feb 1, 2025

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.

@WolframAlph WolframAlph changed the title gh-129552: remove redundant condition inside codegen_addop_load_const gh-129552: Make condition inside codegen_addop_load_const a macro Feb 3, 2025
@WolframAlph
Copy link
Contributor Author

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

WolframAlph and others added 2 commits February 3, 2025 10:31
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
@WolframAlph
Copy link
Contributor Author

WolframAlph commented Feb 3, 2025

Closing this one as it will be done as part of another issue.

@WolframAlph WolframAlph closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants