GeezerVison is showing github.com — dis: Show names of intrinsics · Issue #103963 · python/cpython
dis: Show names of intrinsics #103963DescriptionFor CALL_INTRINSIC_* opcodes, dis currently just shows the number, which makes it hard to figure out what the code actually does:
2 LOAD_CONST 0 ('T')
4 CALL_INTRINSIC_1 7
It would be nice if this instead said
2 LOAD_CONST 0 ('T')
4 CALL_INTRINSIC_1 7 (INTRINSIC_TYPEVAR)
Possible implementation strategy:
- instrinsic names are defined in opcode.py
pycore_intrinsics.h is generated from opcode.py (bonus: we no longer have to manually update MAX_INTRINSIC_1)dis learns to read the intrinsic names from opcode.py
Linked PRs
Activityiritkatriel commented on Apr 28, 2023 jkchandalia commented on Apr 28, 2023 iritkatriel commented on Apr 28, 2023 Great. Have a look at /Tools/build/generate_opcode_h.py
(re generating the .h file). iritkatriel commented on Apr 28, 2023
Great. Have a look at /Tools/build/generate_opcode_h.py
(re generating the .h file).
Use make regen-opcode to run this script. JelleZijlstra commented on Apr 28, 2023 @jkchandalia thanks! It would be nice for me if this can make it in before the 3.12 feature freeze (May 8), since #103764 relies heavily on intrinsics, so this feature will make the bytecode generated by that PR much easier to understand. If that timeline is too tight for you, I'm happy to have a go at implementing this myself. jkchandalia commented on Apr 28, 2023 JelleZijlstra commented on Apr 28, 2023 Great! Let me know if you need any help.
FYI I think these are some of the easiest ways to trigger intrinsics on current main for testing:
>>> dis.dis('(*a,)')
0 0 RESUME 0
1 2 BUILD_LIST 0
4 LOAD_NAME 0 (a)
6 LIST_EXTEND 1
8 CALL_INTRINSIC_1 6
10 RETURN_VALUE
>>> dis.dis('+a')
0 0 RESUME 0
1 2 LOAD_NAME 0 (a)
4 CALL_INTRINSIC_1 5
6 RETURN_VALUE
jkchandalia commented on Apr 28, 2023 Absolutely, thank you. And I appreciate the examples. jkchandalia commented on Apr 30, 2023 @JelleZijlstra I've pushed my branch (https://github.com/jkchandalia/cpython/tree/display_intrinsics_names) and I wasn't quite sure if it's ready for a PR but can definitely open one if that's easier for feedback/discussion.
- I added two lists to
opcode.py for the intrinsics names (I added an extra option for not valid, not sure if this is needed)
- I use these lists in
dis.py to get the argrepr to pass to the Instruction class.
- I added tests for args 2, 5, and 6 for
CALL_INTRINSIC_1 and updated an existing test for arg 3. I put the three new tests into one test function but can split out if better. I had some trouble getting something to trigger the other codes (0?, 1, 4) and also CALL_INTRINSIC_2 for additional testing.
- I updated
/Tools/build/generate_opcode_h.py to include the new intrinsic names sections (not sure if this is desired?)
- I regenerated
opcode.h with the new intrinsics-related sections
- I could definitely write the code to generate
pycore_intrinsics.h as you suggested but I wasn’t quite sure where to put it? A separate script or another internal file output from /Tools/build/generate_opcode_h.py or something else?
JelleZijlstra commented on Apr 30, 2023 Looks great! Feel free to open a draft PR and we can get it over the finish line.
You can get the CALL_INTRINSIC_2 with except* (position 38):
>>> dis.dis("try: pass\nexcept* Exception: x")
0 0 RESUME 0
1 2 RETURN_CONST 0 (None)
4 PUSH_EXC_INFO
2 6 BUILD_LIST 0
8 COPY 2
10 LOAD_NAME 0 (Exception)
12 CHECK_EG_MATCH
14 COPY 1
16 POP_JUMP_IF_NONE 8 (to 34)
18 POP_TOP
20 LOAD_NAME 1 (x)
22 POP_TOP
24 JUMP_FORWARD 3 (to 32)
>> 26 LIST_APPEND 3
28 POP_TOP
30 JUMP_FORWARD 2 (to 36)
>> 32 JUMP_FORWARD 1 (to 36)
>> 34 POP_TOP
>> 36 LIST_APPEND 1
38 CALL_INTRINSIC_2 1
40 COPY 1
42 POP_JUMP_IF_NOT_NONE 3 (to 50)
44 POP_TOP
46 POP_EXCEPT
48 RETURN_CONST 0 (None)
>> 50 SWAP 2
52 POP_EXCEPT
54 RERAISE 0
>> 56 COPY 3
58 POP_EXCEPT
60 RERAISE 1
ExceptionTable:
4 to 18 -> 56 [1] lasti
20 to 22 -> 26 [4] lasti
24 to 44 -> 56 [1] lasti
I don't think it's necessary to test every single one of the intrinsics in test_dis.
I would suggest simply expanding generate_opcode_h.py to also generate pycore_intrinsics.h. It already generates multiple files, so it doesn't seem too bad to make it generate another closely related file. We'll add a header to pycore_intrinsics.h to make it clear how it is generated. Perhaps @iritkatriel has a different opinion, though. We definitely shouldn't add the intrinsics to opcode.h, as that's a public header file and we don't want the intrinsics to be part of the public API. iritkatriel commented on Apr 30, 2023 JelleZijlstra commented on May 3, 2023 Reacted by Juhi Chandalia
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|