Skip to content

dis: Show names of intrinsics #103963

Closed
#104029
Closed
dis: Show names of intrinsics#103963
#104029
Labels
3.12only security fixesstdlibStandard Library Python modules in the Lib/ directorytype-featureA feature request or enhancement

Description

@JelleZijlstra
JelleZijlstra
opened on Apr 28, 2023

For 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

Activity

  1. JelleZijlstra
    added
    type-featureA feature request or enhancement
    3.12only security fixes
    on Apr 28, 2023

  2. iritkatriel commented on Apr 28, 2023

    @iritkatriel
    iritkatriel
    on Apr 28, 2023
    Member

    @jkchandalia are you interested in this?

  3. jkchandalia commented on Apr 28, 2023

    @jkchandalia
    jkchandalia
    on Apr 28, 2023
    Contributor

    @iritkatriel I can work on this.

  4. iritkatriel commented on Apr 28, 2023

    @iritkatriel
    iritkatriel
    on Apr 28, 2023
    Member

    Great. Have a look at /Tools/build/generate_opcode_h.py
    (re generating the .h file).

  5. iritkatriel commented on Apr 28, 2023

    @iritkatriel
    iritkatriel
    on Apr 28, 2023
    Member

    Great. Have a look at /Tools/build/generate_opcode_h.py
    (re generating the .h file).

    Use make regen-opcode to run this script.

  6. iritkatriel
    added
    stdlibStandard Library Python modules in the Lib/ directory
    on Apr 28, 2023

  7. JelleZijlstra commented on Apr 28, 2023

    @JelleZijlstra
    JelleZijlstra
    on Apr 28, 2023
    MemberAuthor

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

  8. jkchandalia commented on Apr 28, 2023

    @jkchandalia
    jkchandalia
    on Apr 28, 2023
    Contributor

    @JelleZijlstra that timeline is fine for me.

  9. JelleZijlstra commented on Apr 28, 2023

    @JelleZijlstra
    JelleZijlstra
    on Apr 28, 2023
    MemberAuthor

    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

  10. jkchandalia commented on Apr 28, 2023

    @jkchandalia
    jkchandalia
    on Apr 28, 2023
    Contributor

    Absolutely, thank you. And I appreciate the examples.

  11. jkchandalia commented on Apr 30, 2023

    @jkchandalia
    jkchandalia
    on Apr 30, 2023
    Contributor

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

    1. 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)
    2. I use these lists in dis.py to get the argrepr to pass to the Instruction class.
    3. 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.
    4. I updated /Tools/build/generate_opcode_h.py to include the new intrinsic names sections (not sure if this is desired?)
    5. I regenerated opcode.h with the new intrinsics-related sections
    6. 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?

  12. JelleZijlstra commented on Apr 30, 2023

    @JelleZijlstra
    JelleZijlstra
    on Apr 30, 2023
    MemberAuthor

    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.

  13. iritkatriel commented on Apr 30, 2023

    @iritkatriel
    iritkatriel
    on Apr 30, 2023
    Member

    Sounds good.

  14. JelleZijlstra
    closed this as completedin #104029on May 3, 2023
  15. JelleZijlstra
    added a commit that references this issue on May 3, 2023

    GH-103963: Make dis display names of args for intrinsics opcodes (#10…

    872cbc6

  16. JelleZijlstra commented on May 3, 2023

    @JelleZijlstra
    JelleZijlstra
    on May 3, 2023
    MemberAuthor

    Thanks for your contribution @jkchandalia!

  17. carljm
    added 3 commits that reference this issue on May 4, 2023

    pythongh-103963: fix 'make regen-opcode' in out-of-tree builds

    84690e7

    gh-103963: fix 'make regen-opcode' in out-of-tree builds (#104177)

    f5c3838

    Merge branch 'main' into inlinecomp2

    8a78a36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    3.12only security fixesstdlibStandard Library Python modules in the Lib/ directorytype-featureA feature request or enhancement

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions