8000 gh-132732: Treat `bytes` as constants in `_Py_uop_sym_is_safe_const` by sobolevn · Pull Request #136033 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-132732: Treat bytes as constants in _Py_uop_sym_is_safe_const #136033

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

gh-132732: Treat bytes as constants in _Py_uop_sym_is_safe_const #136033

wants to merge 2 commits into from

Conversation

sobolevn
Copy link
Member
@sobolevn sobolevn commented Jun 27, 2025 •
edited
Loading 
b'a' + b'b'  # is a constant evaluation

@sobolevn sobolevn requested a review from tomasr8 as a code owner June 27, 2025 12:23
@bedevere-app bedevere-app bot mentioned this pull request Jun 27, 2025
Open
@sobolevn sobolevn mentioned this pull request Jun 27, 2025
Merged
Fidget-Spinner
Fidget-Spinner reviewed Jun 27, 2025
View reviewed changes
Fidget-Spinner
Fidget-Spinner reviewed Jun 27, 2025
View reviewed changes
@brandtbucher
Copy link
Member

One thing that slightly worries me about this is that comparing str/int and bytes objects raises UnicodeWarning if the -b option was passed on the command line. With this change, additional checks are required if we want to make COMPARE_OP able to constant-evaluate in the optimizer.

@Fidget-Spinner
Copy link
Member

One thing that slightly worries me about this is that comparing str/int and bytes objects raises UnicodeWarning if the -b option was passed on the command line. With this change, additional checks are required if we want to make COMPARE_OP able to constant-evaluate in the optimizer.

Ok let's not do this then. We still need to fix the long stuff though so @sobolevn are you open to that or do you want me to handle it?

@Fidget-Spinner
Copy link
Member

I opened PR #136040 to fix the compact ints issue.

@sobolevn
Copy link
Member Author

With this change, additional checks are required if we want to make COMPARE_OP able to constant-evaluate in the optimizer.

Did you mean checking for _Py_GetConfig()->bytes_warning conditionally? I fear that this might be really slow :(

@Fidget-Spinner
Copy link
Member
Fidget-Spinner commented Jun 27, 2025 •
edited
Loading

With this change, additional checks are required if we want to make COMPARE_OP able to constant-evaluate in the optimizer.

Did you mean checking for _Py_GetConfig()->bytes_warning conditionally? I fear that this might be really slow :(

You can fetch the value once at the start of optimization time in optimizer_analysis.c, and store it somewhere.

@brandtbucher
Copy link
Member

Or, simpler, just explicitly disallow optimizing the problematic comparisons.

@brandtbucher
Copy link
Member

If one of the values is of type bytes, require the other to be as well.

@Fidget-Spinner
Copy link
Member

Or, simpler, just explicitly disallow optimizing the problematic comparisons.

Not just comparisons. BINARY_OP as well. This is a little annoying and more involved than I expected, so I'd say let's just ignore it for now?

@brandtbucher
Copy link
Member
brandtbucher commented Jun 27, 2025 •
edited
Loading

What BINARY_OP will raise BytesWarning?

@Fidget-Spinner
Copy link
Member

What BINARY_OP will raise BytesWarning?

Not BytesWarning. BINARY_OP (str, bytes) will throw.

@brandtbucher
Copy link
Member

That’s the case for many combinations of safe types. We should suppress the error and hit bottom in that case (in the error label).

@brandtbucher
Copy link
Member

Same for 42 / 0, etc. In general, errors in the optimizer should just be suppressed.

@brandtbucher
Copy link
Member

This is just different because it’s a warning, to clarify.

@Fidget-Spinner
Copy link
Member

That’s the case for many combinations of safe types. We should suppress the error and hit bottom in that case (in the error label).

Yeah the optimizer currently doesn't do that. Opened a PR to do so #136048

@sobolevn
Copy link
Member Author

I opened https://discuss.python.org/t/consider-deprecating-and-eventually-removing-b-cli-flag/96903 about possibly removing this warning in the future versions. It does not seem very useful :(

@sobolevn
Copy link
Member Author
sobolevn commented Jun 27, 2025 •
edited
Loading

I fixed the merge conflict. Sorry, I am rather new to the JIT internals, and I don't quite understand the COMPARE_OP part. _Py_uop_sym_is_safe_const is never called for COMPARE_OP. So, I don't know how to interpret these comments :)

With this change, additional checks are required if we want to make COMPARE_OP able to constant-evaluate in the optimizer.

Or, simpler, just explicitly disallow optimizing the problematic comparisons.

Is there anything I need to do in this PR to address them?

@Fidget-Spinner
Copy link
Member

I fixed the merge conflict. Sorry, I am rather new to the JIT internals, and I don't quite understand the COMPARE_OP part. _Py_uop_sym_is_safe_const is never called for COMPARE_OP. So, I don't know how to interpret these comments :)

We plan to follow up shortly with COMPARE_OP here #130415

@sobolevn
Copy link
Member Author

Ok then :)
Let's close it for now.

@sobolevn sobolevn closed this Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner left review comments

@tomasr8 tomasr8 tomasr8 approved these changes

@brandtbucher brandtbucher Awaiting requested review from brandtbucher

Assignees
No one assigned
Labels
awaiting merge skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sobolevn @brandtbucher @Fidget-Spinner @tomasr8
0