Skip to content

Conversation

tomasr8
Copy link
Member

@tomasr8 tomasr8 commented Apr 6, 2025

I tested this by finding another instruction which contains a unicode guard, in this case BINARY_OP_ADD_UNICODE and checking that the guard is removed if one of the operands is the result of _BINARY_OP_SUBSCR_STR_INT. Let me know if there's a better way to test this!

I also noticed that some other uops like _BINARY_OP_ADD_UNICODE check if the operands are constants. Should we add it here as well? I don't know if we gain much from that though, code like 'foo'[0] is not very common.

@Fidget-Spinner
Copy link
Member

I also noticed that some other uops like _BINARY_OP_ADD_UNICODE check if the operands are constants. Should we add it here as well? I don't know if we gain much from that though, code like 'foo'[0] is not very common.

The constant checking is sort of a relic. We don't even do anything with it. So it's fine to leave it out for now.

@python-cla-bot
Copy link

python-cla-bot bot commented Apr 6, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@tomasr8 tomasr8 requested a review from brandtbucher April 6, 2025 17:26
@brandtbucher
Copy link
Member

brandtbucher commented Apr 8, 2025

I don't know if we gain much from that though, code like 'foo'[0] is not very common.

Just a quick note on this: "constant" here means anything the JIT knows to be a constant value at runtime, not just things that are statically constant in the source. So it could be something like MODULE_GLOBAL[len(foo) - 1], for instance.

The constant checking is sort of a relic. We don't even do anything with it.

I disagree, constant symbols are pretty powerful, and we should be using them more. For example, knowing constant values currently allows us to entirely remove conditional branches, type version checks, etc. @savannahostrowski also added the new _POP_TWO_LOAD_CONST_INLINE_BORROW instruction (and is in the process of using it in the remaining _BINARY_OP_* and _COMPARE_OP_* uops, which allows us to remove these operations entirely when the result is immortal).

I agree that it's probably not worth adding to this uop specifically (at least not without some examples of real-world code it would optimize), but I do think that tracking constant values in general is a good thing.

Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Want to tidy up the test before I merge?

@brandtbucher brandtbucher self-assigned this Apr 8, 2025
@brandtbucher brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-JIT labels Apr 8, 2025
Co-authored-by: Brandt Bucher <[email protected]>
@tomasr8
Copy link
Member Author

tomasr8 commented Apr 8, 2025

Thanks for the clarification about the meaning of constants in the JIT! That's an important distinction to make for sure.

@brandtbucher brandtbucher merged commit 71009cb into python:main Apr 8, 2025
58 checks passed
@tomasr8 tomasr8 deleted the jit-narrow-binary-subscr branch April 8, 2025 15:50
seehwan pushed a commit to seehwan/cpython that referenced this pull request Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage topic-JIT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants