Skip to content

gh-101799: implement PREP_RERAISE_STAR as an intrinsic function #101800

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

Merged
merged 5 commits into from
Feb 14, 2023

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Feb 10, 2023

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Quick comment: Why not add a CALL_INTRINSIC_2 opcode (assuming we expect more of these)? The variable stack effect based on comparison with an arbitrary constant feels a little fragile. Maybe it would feel less arbitrary if the constant was actually 128 (i.e., bit 7, 0x80).

@iritkatriel
Copy link
Member Author

Quick comment: Why not add a CALL_INTRINSIC_2 opcode (assuming we expect more of these)? The variable stack effect based on comparison with an arbitrary constant feels a little fragile. Maybe it would feel less arbitrary if the constant was actually 128 (i.e., bit 7, 0x80).

I considered this, the down side is we have two opcodes for non-perf-critical stuff. I don't have a strong opinion either way. @markshannon ?

@iritkatriel
Copy link
Member Author

Is FORMAT_VALUE performance critical? It could be replaced by a couple of intrinsic functions.

@gvanrossum
Copy link
Member

Is FORMAT_VALUE performance critical? It could be replaced by a couple of intrinsic functions.

Possibly. Most of it could become a helper function. This would lose the fast path for f"aaa{foo}zzz" if foo is exactly a Unicode string, but I'm not sure how important that really is.

@markshannon
Copy link
Member

Usually we want to do dispatching in the dispatch loop, not within instructions.
However, the point of the CALL_INTRINSIC instruction is to reduce the umber of instructions, so its less clear cut.

On balance, I still think we want two instructions. The extra test and branch is messy; two instructions would make for clearer code and we aren't that short of opcodes.

@markshannon
Copy link
Member

Regarding FORMAT_VALUE, I refer you to the venerable #6132 which split FORMAT_VALUE into CONVERT_VALUE, FORMAT_SIMPLE and FORMAT_WITH_SPEC.

CONVERT_VALUE could be replaced with CALL_INSTINSIC_1 and FORMAT_WITH_SPEC could be replaced with CALL_INSTINSIC_2. FORMAT_SIMPLE is simple enough, and probably common enough, that it merits its own instruction.

Copy link
Member

@markshannon markshannon 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants