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

 _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

 }

 caseTO_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

 _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 <[email protected]>

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

 "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

 }

 caseTO_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

+iffamily.namenotinself.macro_instrsandfamily.namenotinself.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_filehttps://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_filehttps://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_filehttps://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_filehttps://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-106706Streamline 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-itbotand 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

kgdiemand 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 <[email protected]>

Remove redundant checks for family member length
906c8b2

Reword documentation
c367600

Update test, regenerate headers
d31258c

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

Merge branch 'main' into streamline-family-syntax
178e300

erlend-aasland removed request for AlexWaygood and erlend-aasland 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:mainJul 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).

Uh oh!

gh-106706: Streamline family syntax#106716

gh-106706: Streamline family syntax #106716

Uh oh!

Conversation

kgdiem commented Jul 13, 2023• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Jul 13, 2023

Uh oh!

bedevere-bot commented Jul 13, 2023

Uh oh!

kgdiemJul 13, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 13, 2023

Uh oh!

bedevere-bot commented Jul 13, 2023

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgdiem commented Jul 15, 2023

Uh oh!

gvanrossum commented Jul 15, 2023

Uh oh!

kgdiem commented Jul 15, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jul 15, 2023

Uh oh!

kgdiem commented Jul 15, 2023 via email

Uh oh!

kgdiem commented Jul 16, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kgdiem commented Jul 16, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kgdiem commented Jul 16, 2023

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

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

kgdiemJul 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