gh-118423: Add `INSTRUCTION_SIZE` macro to code generator #125467

tomasr8 · 2024-10-14T15:42:23Z

I am not that familiar with the tier 1 interpreter (yet 😄 ) so I'd appreciate if someone could double check that for all
the places that use the new INSTRUCTION_SIZE macro, it is actually safe to do so.

I replaced all examples of next_instr - this_instr. There are still a few examples of 1 + INLINE_CACHE_ENTRIES_... but I wasn't sure whether those were safe to change as well.

I named the new macro INSTRUCTION_SIZE, but I'm happy to change it to INSTRUCTION_LENGTH if you prefer.

Feedback welcome :)

Issue: Add INSTRUCTION_SIZE macro to code generator. #118423

tomasr8 · 2024-10-14T15:43:35Z

cc @iritkatriel since you asked for this in #118380 (comment) :)

Tools/cases_generator/generators_common.py

Fidget-Spinner

This is really close to what I was thinking of. Great work!

Tools/cases_generator/lexer.py

tomasr8 · 2024-10-14T19:19:57Z

Tools/cases_generator/generators_common.py

+        inst: Instruction | None
+    ) -> bool:
+        """Replace the INSTRUCTION_SIZE macro with the size of the current instruction."""
+        size = inst.size if inst else 1 + uop.size


tier2 does not have access to the instruction here so I calculate the size as 1 + uop.size. Does that make sense?

Sorry this seems wrong, you need to emit the 1 + inline cache size not uop size

Indeed, I see when comparing the output of generated_cases.c.h with executor_cases.c.h that the latter has the wrong instruction size. I fixed it (I think) by looking up the instructions for each uop in tier2 and passing it to the emitter.
Let me know if it's correct like that :)

INSTRUCTION_SIZE is the size of the tier 1 instruction. This must be true even in tier 2.

I fixed it (I think) by looking up the instructions for each uop in tier2 and passing it to the emitter.

That is correct, but it would be nice if we also checked that all matching tier 1 instructions are the same size.

Tools/cases_generator/generators_common.py

tomasr8 · 2024-10-16T19:27:21Z

It looks like there are not tests for tier2 yet so I only added tests for tier1

markshannon

Thanks for doing this.

The general approach looks good, but I think we need to check that all the instructions containing the uop with INSTRUCTION_LENGTH are the same length.

markshannon · 2024-10-17T10:42:08Z

Lib/test/test_generated_cases.py

+        }
+        """
+        self.run_cases_test(input, output)
+


Add a test for another instruction that is a different length, but uses OP:

macro(OP2) = unused/1 + OP;

That test should fail, as the INSTRUCTION_SIZE is inconsistent.
The tier 2 INSTRUCTION_SIZE cannot be both 1 and 2.

Right, sorry I mistakenly thought it should only fail for tier 2 hence I was only checking the instruction in the tier 2 generator. I extended the check to tier 1 as well and the test now fails as expected.

Tools/cases_generator/generators_common.py

markshannon · 2024-10-17T10:47:37Z

Tools/cases_generator/tier2_generator.py

@@ -167,7 +167,7 @@ def oparg(
        return True


-def write_uop(uop: Uop, emitter: Emitter, stack: Stack) -> Stack:
+def write_uop(uop: Uop, emitter: Emitter, stack: Stack, inst: Instruction | None) -> Stack:


Maybe pass the size, not the instruction. A uop can occur in many instructions, so picking one is arbitrary.
However, if it uses INSTRUCTION_SIZE, than all instructions must be the same size.

I added the check for the instruction size, which actually pointed out one place where we cannot use INSTRUCTION_SIZE instead of next_instr - this_instr:

cpython/Python/bytecodes.c

Line 4799 in d8c8648

frame->return_offset = (uint16_t)(next_instr - this_instr);

bedevere-app · 2024-10-17T10:49:21Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

tomasr8 · 2024-10-21T21:42:06Z

Passing the instruction size or None to every replacement function seems a bit clunky though.
Maybe would could add an instruction_size attribute to the Uop class and do the instruction size checking in analyzer.py?

I also thought it was less than ideal the way I did it, but I was a bit wary about adding more attributes to the classes. Though, your suggestion simplifies the code quite a bit!

I added a new attribute Uop.instruction_size which is only set for uops which contain the macro, otherwise it is None. This attribute is then used by the emitter.

tomasr8 · 2024-10-26T15:39:46Z

Friendly ping @markshannon 🙂 I think I addressed all of your points.

markshannon

The assertion needs to be changed to explicitly raise. Otherwise it won't work with -O enabled.

Lib/test/test_generated_cases.py

Tools/cases_generator/analyzer.py

bedevere-app · 2024-10-28T09:42:15Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

tomasr8 · 2024-10-28T21:07:36Z

Thanks again for the review!

I have made the requested changes; please review again 🙂

bedevere-app · 2024-10-28T21:07:42Z

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

markshannon

This looks good and correct now. Thanks.

I have a few suggestions, but they are mostly cosmetic.

Tools/cases_generator/analyzer.py

markshannon · 2024-10-29T10:29:29Z

Tools/cases_generator/analyzer.py

+        if uop in inst.parts:
+            if size is None:
+                size = inst.size
+            if size != inst.size:


Suggested change

if size != inst.size:

elif size != inst.size:

I changed this to an if because otherwise mypy complains that the elif is unreachable:

Tools/cases_generator/analyzer.py:1103: error: Statement is unreachable [unreachable]

OK, leave it then.

markshannon

Looks good.
Thanks for doing this.

tomasr8 · 2024-10-29T21:21:07Z

Thanks for all the reviews! :)

tomasr8 added 6 commits October 14, 2024 17:11

Add INSTRUCTION_SIZE macro

ec8c701

Use INSTRUCTION_SIZE in bytecodes.c

09c2f59

Add tests

287eb92

Update docs

c76fce4

Add news entry

e3f3b07

Fix mypy error

ed5b375

tomasr8 requested a review from markshannon as a code owner October 14, 2024 15:42

bedevere-app bot mentioned this pull request Oct 14, 2024

Add INSTRUCTION_SIZE macro to code generator. #118423

Open

bedevere-app bot added the awaiting review label Oct 14, 2024

Fidget-Spinner reviewed Oct 14, 2024

View reviewed changes

Tools/cases_generator/generators_common.py Outdated Show resolved Hide resolved

Fidget-Spinner reviewed Oct 14, 2024

View reviewed changes

Tools/cases_generator/lexer.py Outdated Show resolved Hide resolved

tomasr8 added 3 commits October 14, 2024 20:54

Treat INSTRUCTION_SIZE as an identifier

870341d

Regenerate executor cases

421ebda

Re-add newline

5154f7f

tomasr8 commented Oct 14, 2024

View reviewed changes

Use replaceText()

e7e83dc

Fidget-Spinner reviewed Oct 15, 2024

View reviewed changes

Tools/cases_generator/generators_common.py Outdated Show resolved Hide resolved

tomasr8 added 5 commits October 15, 2024 22:16

Emit size directly

101a5fd

Merge branch 'main' into instr-size

1338628

Pass instruction in tier2

ee461d5

Fix INSTRUCTION_SIZE in tier2

7135103

Make mypy happy

715ce31

Fix tests

8235321

markshannon requested changes Oct 17, 2024

View reviewed changes

bedevere-app bot removed the awaiting review label Oct 17, 2024

bedevere-app bot added the awaiting changes label Oct 17, 2024

Make mypy happy

3f3ad10

markshannon self-requested a review October 28, 2024 09:40

markshannon requested changes Oct 28, 2024

View reviewed changes

bedevere-app bot removed the awaiting change review label Oct 28, 2024

bedevere-app bot added the awaiting changes label Oct 28, 2024

tomasr8 added 4 commits October 28, 2024 19:32

Simplify test

a8f72ef

Replace assert with raise

8138a06

Simplify code

772f803

Remove some code repetition

a718cfb

bedevere-app bot added awaiting change review and removed awaiting changes labels Oct 28, 2024

bedevere-app bot requested a review from markshannon October 28, 2024 21:07

markshannon reviewed Oct 29, 2024

View reviewed changes

tomasr8 added 2 commits October 29, 2024 17:49

Simplify INSTRUCTION_SIZE check

a10d4ee

Simplify code

3ae7a5d

markshannon requested review from markshannon and Fidget-Spinner October 29, 2024 17:16

markshannon approved these changes Oct 29, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting change review labels Oct 29, 2024

markshannon enabled auto-merge (squash) October 29, 2024 17:17

markshannon merged commit aab58a9 into python:main Oct 29, 2024
70 of 71 checks passed

bedevere-app bot removed the awaiting merge label Oct 29, 2024

tomasr8 deleted the instr-size branch October 29, 2024 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-118423: Add `INSTRUCTION_SIZE` macro to code generator #125467

gh-118423: Add `INSTRUCTION_SIZE` macro to code generator #125467

tomasr8 commented Oct 14, 2024 •

edited by bedevere-app bot

Loading

tomasr8 commented Oct 14, 2024

Fidget-Spinner left a comment

tomasr8 Oct 14, 2024

Fidget-Spinner Oct 16, 2024

tomasr8 Oct 16, 2024

markshannon Oct 17, 2024

tomasr8 commented Oct 16, 2024

markshannon left a comment

markshannon Oct 17, 2024

tomasr8 Oct 17, 2024

markshannon Oct 19, 2024

tomasr8 Oct 20, 2024

markshannon Oct 17, 2024

tomasr8 Oct 17, 2024

bedevere-app bot commented Oct 17, 2024

tomasr8 commented Oct 21, 2024

tomasr8 commented Oct 26, 2024

markshannon left a comment

bedevere-app bot commented Oct 28, 2024

tomasr8 commented Oct 28, 2024

bedevere-app bot commented Oct 28, 2024

markshannon left a comment

markshannon Oct 29, 2024

tomasr8 Oct 29, 2024

markshannon Oct 29, 2024

markshannon left a comment

tomasr8 commented Oct 29, 2024

gh-118423: Add INSTRUCTION_SIZE macro to code generator #125467

gh-118423: Add INSTRUCTION_SIZE macro to code generator #125467

Conversation

tomasr8 commented Oct 14, 2024 • edited by bedevere-app bot Loading

tomasr8 commented Oct 14, 2024

Fidget-Spinner 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

tomasr8 commented Oct 16, 2024

markshannon 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-app bot commented Oct 17, 2024

tomasr8 commented Oct 21, 2024

tomasr8 commented Oct 26, 2024

markshannon left a comment

Choose a reason for hiding this comment

bedevere-app bot commented Oct 28, 2024

tomasr8 commented Oct 28, 2024

bedevere-app bot commented Oct 28, 2024

markshannon 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

markshannon left a comment

Choose a reason for hiding this comment

tomasr8 commented Oct 29, 2024

gh-118423: Add `INSTRUCTION_SIZE` macro to code generator #125467

gh-118423: Add `INSTRUCTION_SIZE` macro to code generator #125467

tomasr8 commented Oct 14, 2024 •

edited by bedevere-app bot

Loading