gh-106706: Streamline family syntax #106716

kgdiem · 2023-07-13T11:46:32Z

Update bytecodes.c to use suggested syntax from Streamline family syntax in Tools/cases_generator #106706
Update checks in generate_cases.py to support >= 1 members and validate family name
Update documentation
Update tests

Issue: Streamline family syntax in Tools/cases_generator #106706

bedevere-bot · 2023-07-13T11:46:37Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

bedevere-bot · 2023-07-13T11:46:47Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

kgdiem · 2023-07-13T11:51:21Z

Tools/cases_generator/test_generator.py

@@ -321,13 +321,13 @@ def test_macro_instruction():
                _tmp_3 = res;
            }
            next_instr += 5;
-            static_assert(INLINE_CACHE_ENTRIES_OP == 5, "incorrect cache size");


It isn't clear to me how/where the INLINE_CACHE_ENTRIES_OP instruction is generated.

Changing the test to pass w/o understanding doesn't inspire confidence on my end so any guidance or explanation here is greatly appreciated.

It is the second parameter to family.

bedevere-bot · 2023-07-13T11:59:22Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

bedevere-bot · 2023-07-13T12:00:01Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

gvanrossum

Thank you! I hope I've given you enough hints on what to do about the static assert -- if not, let me know.

Misc/NEWS.d/next/Tools-Demos/2023-07-13-12-08-35.gh-issue-106706.29zp8E.rst

gvanrossum · 2023-07-14T00:54:14Z

Python/executor_cases.c.h

@@ -183,6 +182,7 @@
        }

        case TO_BOOL_ALWAYS_TRUE: {
+            static_assert(INLINE_CACHE_ENTRIES_TO_BOOL == 3, "incorrect cache size");


While it works, I think it's better to keep the static assert in the "family head".

This still isn't resolved?

No, I think this might be related to the change in Lib/_opcode_metadata.pyas well; the op from the family name isn't getting inserted into instrs

I'm having some trouble getting the name into instrs / parsed as an instruction. Any advice?

Tools/cases_generator/generate_cases.py

Tools/cases_generator/interpreter_definition.md

gvanrossum · 2023-07-14T01:00:07Z

Tools/cases_generator/test_generator.py

@@ -321,13 +321,13 @@ def test_macro_instruction():
                _tmp_3 = res;
            }
            next_instr += 5;
-            static_assert(INLINE_CACHE_ENTRIES_OP == 5, "incorrect cache size");


It is the second parameter to family.

…06.29zp8E.rst Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>

gvanrossum

I think you pushed a rebase? Please don't do that once review is in progress -- everything will be squashed when we merge into main.

Can you look into moving the static_assert back to the family head?

And I'm unsure why the metadata file was truncated. (Let me know if you need help figuring this out.)

gvanrossum · 2023-07-14T21:02:08Z

Lib/_opcode_metadata.py

@@ -31,75 +31,3 @@
        "STORE_SUBSCR_DICT",
        "STORE_SUBSCR_LIST_INT",
    ],
-    "SEND": [


That this file changed surprises me. Maybe this points to a problem?

It didn't change originally, I think only after 178e300 ?

gvanrossum · 2023-07-14T21:04:13Z

Python/executor_cases.c.h

@@ -183,6 +182,7 @@
        }

        case TO_BOOL_ALWAYS_TRUE: {
+            static_assert(INLINE_CACHE_ENTRIES_TO_BOOL == 3, "incorrect cache size");


This still isn't resolved?

gvanrossum · 2023-07-14T21:04:29Z

Tools/cases_generator/generate_cases.py

+            if family.name not in self.macro_instrs and family.name not in self.instrs:
+                self.error(
+                    f"Family {family.name!r} has unknown instruction {family.name!r}",
+                    family,
+                )


Good catch!

kgdiem · 2023-07-15T04:41:46Z

I think you pushed a rebase? Please don't do that once review is in progress -- everything will be squashed when we merge into main.

Can you look into moving the static_assert back to the family head?

And I'm unsure why the metadata file was truncated. (Let me know if you need help figuring this out.)

Ya there was a conflict in one of the header files so I rebased and re-generated it. Will keep that in mind next time.

gvanrossum · 2023-07-15T16:02:46Z

Ya there was a conflict in one of the header files so I rebased and re-generated it. Will keep that in mind next time.

The missing part of the metadata will be reconstructed if you simply run make regen-cases and then commit the resulting changes to Lib/_opcode_metadata.py.

Not sure yet why the stat asserts moved, will look into it later. (Did you find where they are generated?)

kgdiem · 2023-07-15T16:22:03Z

Ya there was a conflict in one of the header files so I rebased and re-generated it. Will keep that in mind next time.

The missing part of the metadata will be reconstructed if you simply run make regen-cases and then commit the resulting changes to Lib/_opcode_metadata.py.

Not sure yet why the stat asserts moved, will look into it later. (Did you find where they are generated?)

Yes, https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L1555

I had changed this on my local from .members[0] to family.name but:

This condition is always falsy because last_instr.family is None on the instruction used as family.name.

That instruction misses in map_families (https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L835) because it isn't added to the instrs in parse_file https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L787

I'm thinking that there would be far fewer changes to generate_cases while keeping the desired syntax for a family template if the family name was added to the members here https://github.com/python/cpython/blob/main/Tools/cases_generator/parser.py#L347

eg ..., [tkn.txt] + members

I considered just starting fresh from there but thought I would check given we've been back and forth on this implementation for a couple days.

gvanrossum · 2023-07-15T18:15:18Z

I'm thinking that there would be far fewer changes to generate_cases while keeping the desired syntax for a family template if the family name was added to the members here https://github.com/python/cpython/blob/main/Tools/cases_generator/parser.py#L347

eg ..., [tkn.txt] + members

I considered just starting fresh from there but thought I would check given we've been back and forth on this implementation for a couple days.

Interesting idea. It would mean fewer changes, but some of the ugliness would remain as well (e.g. the several places where you have to slice .members[1:]).

As you point out, the problem is that the family head doesn't have self.family set to the family whose head it is. I think this will fix it:

diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py
index 48c082a429..4756f50b8f 100644
--- a/Tools/cases_generator/generate_cases.py
+++ b/Tools/cases_generator/generate_cases.py
@@ -438,7 +438,7 @@ def write(self, out: Formatter, tier: Tiers = TIER_ONE) -> None:
         """Write one instruction, sans prologue and epilogue."""
         # Write a static assertion that a family's cache size is correct
         if family := self.family:
-            if self.name == family.members[0]:
+            if self.name == family.name:
                 if cache_size := family.size:
                     out.emit(
                         f"static_assert({cache_size} == "
@@ -831,7 +831,7 @@ def find_predictions(self) -> None:
     def map_families(self) -> None:
         """Link instruction names back to their family, if they have one."""
         for family in self.families.values():
-            for member in family.members:
+            for member in [family.name] + family.members:
                 if member_instr := self.instrs.get(member):
                     if member_instr.family not in (family, None):
                         self.error(

kgdiem · 2023-07-15T18:51:17Z

Awesome! I’ll get everything cleaned up in a little bit.

…

On Sat, Jul 15, 2023 at 14:15 Guido van Rossum ***@***.***> wrote: I'm thinking that there would be far fewer changes to generate_cases while keeping the desired syntax for a family template if the family name was added to the members here https://github.com/python/cpython/blob/main/Tools/cases_generator/parser.py#L347 eg ..., [tkn.txt] + members I considered just starting fresh from there but thought I would check given we've been back and forth on this implementation for a couple days. Interesting idea. It would mean fewer changes, but some of the ugliness would remain as well (e.g. the several places where you have to slice .members[1:]). As you point out, the problem is that the family head doesn't have self.family set to the family whose head it is. I think this will fix it: diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index 48c082a429..4756f50b8f 100644--- a/Tools/cases_generator/generate_cases.py+++ b/Tools/cases_generator/generate_cases.py@@ -438,7 +438,7 @@ def write(self, out: Formatter, tier: Tiers = TIER_ONE) -> None: """Write one instruction, sans prologue and epilogue.""" # Write a static assertion that a family's cache size is correct if family := self.family:- if self.name == family.members[0]:+ if self.name == family.name: if cache_size := family.size: out.emit( f"static_assert({cache_size} == "@@ -831,7 +831,7 @@ def find_predictions(self) -> None: def map_families(self) -> None: """Link instruction names back to their family, if they have one.""" for family in self.families.values():- for member in family.members:+ for member in [family.name] + family.members: if member_instr := self.instrs.get(member): if member_instr.family not in (family, None): self.error( — Reply to this email directly, view it on GitHub <#106716 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADEAQ5K6WG2BKIGMCJN7VFDXQLM4FANCNFSM6AAAAAA2I2HEUA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

…name to map

kgdiem · 2023-07-16T10:31:37Z

This condition is always falsy because last_instr.family is None on the instruction used as family.name.

That instruction misses in map_families (https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L835) because it isn't added to the instrs in parse_file https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L787

I committed those changes but I'd actually outlined them in my previous comment:

This condition is always falsy because last_instr.family is None on the instruction used as family.name.

That instruction misses in map_families (https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L835) because it isn't added to the instrs in parse_file https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L787

family.name is never mapped because it isn't present in self.instrs; This results in the assertion never being emitted.

kgdiem · 2023-07-16T10:45:46Z

This condition is always falsy because last_instr.family is None on the instruction used as family.name.
That instruction misses in map_families (https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L835) because it isn't added to the instrs in parse_file https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L787

I committed those changes but I'd actually outlined them in my previous comment:

This condition is always falsy because last_instr.family is None on the instruction used as family.name.
That instruction misses in map_families (https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L835) because it isn't added to the instrs in parse_file https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L787

family.name is never mapped because it isn't present in self.instrs; This results in the assertion never being emitted.

Looks like the generated headers are now correct / unchanged and the test is testing for a macro instruction and what I described above is still missing here https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L1555

But the generated header files after that change are correct.

Makes a lot more sense now why the macro isn't in self.instrs, wasn't thinking about it correctly before. Still unsure of what to do or if the test is wrong.

kgdiem · 2023-07-16T11:15:18Z

This condition is always falsy because last_instr.family is None on the instruction used as family.name.
That instruction misses in map_families (https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L835) because it isn't added to the instrs in parse_file https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L787

I committed those changes but I'd actually outlined them in my previous comment:

This condition is always falsy because last_instr.family is None on the instruction used as family.name.
That instruction misses in map_families (https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L835) because it isn't added to the instrs in parse_file https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L787

family.name is never mapped because it isn't present in self.instrs; This results in the assertion never being emitted.

Looks like the generated headers are now correct / unchanged and the test is testing for a macro instruction and what I described above is still missing here https://github.com/python/cpython/blob/main/Tools/cases_generator/generate_cases.py#L1555

But the generated header files after that change are correct.

Makes a lot more sense now why the macro isn't in self.instrs, wasn't thinking about it correctly before. Still unsure of what to do or if the test is wrong.

Got it! Instead of checking the last instruction, get the family from self.families

gvanrossum

Perfect! I'll merge.

These repair nits I found in PR gh-106798 (issue gh-106797) and in PR gh-106716 (issue gh-106706).

kgdiem added 10 commits July 13, 2023 06:40

pythongh-106706 Streamline family syntax

294750d

Update syntax in interpreter_definition.md

7dc364c

Further refactor bytecodes.c

dac8eb5

Update test

b05a7f8

Update syntax in interpreter_definition.md

47f2491

Update interpreter_definition to new syntax

d499926

Use family name as first instruction

6fee03d

stop using slice in iters

d13dbf2

Remove formatting in interpreter_definition.md

105871c

Change test

945f09d

bedevere-bot added the awaiting review label Jul 13, 2023

Merge branch 'main' into streamline-family-syntax

c4ffdc5

bedevere-bot mentioned this pull request Jul 13, 2023

Streamline family syntax in Tools/cases_generator #106706

Closed

kgdiem commented Jul 13, 2023

View reviewed changes

Fix check

b588c72

Fix assertion

e091ca3

blurb-it bot and others added 3 commits July 13, 2023 12:08

📜🤖 Added by blurb_it.

59dedfe

lint blurb

868ae1b

Merge branch 'main' into streamline-family-syntax

ae5cd52

gvanrossum self-requested a review July 14, 2023 00:50

gvanrossum added the skip news label Jul 14, 2023

gvanrossum reviewed Jul 14, 2023

View reviewed changes

kgdiem and others added 4 commits July 14, 2023 06:55

Update Misc/NEWS.d/next/Tools-Demos/2023-07-13-12-08-35.gh-issue-1067…

843880d

…06.29zp8E.rst Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>

Remove redundant checks for family member length

906c8b2

Reword documentation

c367600

Update test, regenerate headers

d31258c

kgdiem requested review from erlend-aasland, AlexWaygood, barneygale and markshannon as code owners July 14, 2023 11:38

Merge branch 'main' into streamline-family-syntax

178e300

erlend-aasland removed request for erlend-aasland and AlexWaygood July 14, 2023 11:47

gvanrossum reviewed Jul 14, 2023

View reviewed changes

barneygale removed their request for review July 15, 2023 16:25

Regen cases

eab1974

Check against family name when emitting cache size check, add family.…

1e68564

…name to map

kgdiem added 2 commits July 16, 2023 06:38

update family syntax in test

85cbbed

Regenerate cases

82a9927

Check families for mac.name instead of last_instr.family

b05e24d

kgdiem requested a review from gvanrossum July 16, 2023 11:15

gvanrossum approved these changes Jul 16, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jul 16, 2023

gvanrossum merged commit cc25ca1 into python:main Jul 16, 2023

bedevere-bot removed the awaiting merge label Jul 16, 2023

gvanrossum mentioned this pull request Jul 18, 2023

Small fixes to code generator #106845

Merged

gvanrossum added a commit that referenced this pull request Jul 18, 2023

Small fixes to code generator (#106845)

1e36ca6

These repair nits I found in PR gh-106798 (issue gh-106797) and in PR gh-106716 (issue gh-106706).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-106706: Streamline family syntax #106716

gh-106706: Streamline family syntax #106716

kgdiem commented Jul 13, 2023 •

edited by bedevere-bot

Loading

bedevere-bot commented Jul 13, 2023

bedevere-bot commented Jul 13, 2023

kgdiem Jul 13, 2023 •

edited

Loading

gvanrossum Jul 14, 2023

bedevere-bot commented Jul 13, 2023

bedevere-bot commented Jul 13, 2023

gvanrossum left a comment

gvanrossum Jul 14, 2023

gvanrossum Jul 14, 2023

kgdiem Jul 15, 2023

gvanrossum Jul 14, 2023

gvanrossum left a comment

gvanrossum Jul 14, 2023

kgdiem Jul 15, 2023

gvanrossum Jul 14, 2023

gvanrossum Jul 14, 2023

kgdiem commented Jul 15, 2023

gvanrossum commented Jul 15, 2023

kgdiem commented Jul 15, 2023 •

edited

Loading

gvanrossum commented Jul 15, 2023

kgdiem commented Jul 15, 2023 via email

kgdiem commented Jul 16, 2023 •

edited

Loading

kgdiem commented Jul 16, 2023 •

edited

Loading

kgdiem commented Jul 16, 2023

gvanrossum left a comment

gh-106706: Streamline family syntax #106716

gh-106706: Streamline family syntax #106716

Conversation

kgdiem commented Jul 13, 2023 • edited by bedevere-bot Loading

bedevere-bot commented Jul 13, 2023

bedevere-bot commented Jul 13, 2023

kgdiem Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jul 13, 2023

bedevere-bot commented Jul 13, 2023

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgdiem commented Jul 15, 2023

gvanrossum commented Jul 15, 2023

kgdiem commented Jul 15, 2023 • edited Loading

gvanrossum commented Jul 15, 2023

kgdiem commented Jul 15, 2023 via email

kgdiem commented Jul 16, 2023 • edited Loading

kgdiem commented Jul 16, 2023 • edited Loading

kgdiem commented Jul 16, 2023

gvanrossum left a comment

Choose a reason for hiding this comment

kgdiem commented Jul 13, 2023 •

edited by bedevere-bot

Loading

kgdiem Jul 13, 2023 •

edited

Loading

kgdiem commented Jul 15, 2023 •

edited

Loading

kgdiem commented Jul 16, 2023 •

edited

Loading

kgdiem commented Jul 16, 2023 •

edited

Loading