GH-111485: Break up instructions with unused cache entries into component micro-ops by markshannon · Pull Request #113169 · python/cpython

markshannon · 2023-12-15T13:55:03Z

Break up inst into more uops if they contain unused cache entries.

Ideally, micro-ops should not have unused cache entries. This PR implements that for inst declarations.

For example:

inst(TO_BOOL_BOOL, (unused/1, unused/2, value -- value)) {
    DEOPT_IF(!PyBool_Check(value));
    STAT_INC(TO_BOOL, hit);
}

currently converts to:

op(_TO_BOOL_BOOL, (unused/1, unused/2, value -- value)) ...
macro(TO_BOOL_BOOL) = _TO_BOOL_BOOL

With this PR it becomes:

op(_TO_BOOL_BOOL, (value -- value)) ...
macro(TO_BOOL_BOOL) = unused/1, unused/2, _TO_BOOL_BOOL

Issue: The code generated for the interpreter(s) is too fragile #111485

iritkatriel · 2023-12-15T14:37:07Z

Lib/test/test_generated_cases.py

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


I'm not sure this test is covering everything that changed in this PR.

Probably not, it is just a smoke test.
The real test is that everything continues to work properly.

Actually, it would be nice if there was a test that adds a /* Skip N cache entries */ comment where it wasn't before this PR, so we could see regressions around that. (I don't care about the singular/plural, but I care about it appearing at all.)

iritkatriel · 2023-12-15T14:37:51Z

Python/generated_cases.c.h

            PyObject *self;
            PyObject *callable;
+            /* Skip 1 cache entry */
+            /* Skip 2 cache entries */


Can it print only that second line?

The source is inst(CALL_LIST_APPEND, (unused/1, unused/2, callable, self, args[oparg] -- unused))
So it is skipping 1 entry, then skipping 2 entries.
We could combine them into /* Skip 3 cache entries */ but that would lose the structure, and I don't think it is worth the effort.

gvanrossum

LG except I am mystified by the logic for unused in desugar_inst().

gvanrossum · 2023-12-16T05:42:43Z

Python/generated_cases.c.h

            static_assert(INLINE_CACHE_ENTRIES_UNPACK_SEQUENCE == 1, "incorrect cache size");
            PyObject *seq;
            PyObject **values;
+            /* Skip 1 cache entry */


In summary, this PR doesn't change the generated cases except by adding some comments.

gvanrossum · 2023-12-16T05:43:58Z

Lib/test/test_generated_cases.py

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


Actually, it would be nice if there was a test that adds a /* Skip N cache entries */ comment where it wasn't before this PR, so we could see regressions around that. (I don't care about the singular/plural, but I care about it appearing at all.)

gvanrossum · 2023-12-16T05:56:03Z

Tools/cases_generator/analyzer.py

+    op_inputs: list[parser.InputEffect] = []
+    parts: list[Part] = []
+    uop_index = -1
+    for input in inst.inputs:
+        if isinstance(input, parser.CacheEffect) and input.name == "unused":
+            parts.append(Skip(input.size))
+        else:
+            op_inputs.append(input)
+            if uop_index < 0:
+                uop_index = len(parts)
+                parts.append(Skip(0))
+    uop = make_uop("_" + inst.name, inst, op_inputs)
    uop.implicitly_created = True
    uops[inst.name] = uop
-    add_instruction(name, [uop], instructions)
+    if uop_index < 0:
+        parts.append(uop)
+    else:
+        parts[uop_index] = uop
+    add_instruction(name, parts, instructions)


Something here feels so clever it needs a comment. and/or an assert We're adding a Skip(0) in L360 and overwriting that in L367. Could we ever add multiple Skip(0) entries? Then we'd be overwriting the first Skip(0) only. Probably my question makes no sense, and a comment would clarify that.

Separately, maybe Skip should have been named Unused? Or even UnusedCacheEffect? Because that's what it represents, right?

We have to add Skip(0) due to type constraints. The natural thing to add would be None, but the then we need to add casts, or messy types.

Skip skips over caches entries, so I think the name is better than Unused. We could add Cache though.
A bit out of scope for this PR, though.

… component micro-ops (pythonGH-113169)

markshannon added 8 commits December 14, 2023 05:10

Break up instructions with unused cache entries into component micro-ops

8dc1a32

Fix whitespace

182309e

Test new cases generator, not old one

6a0b2e0

Test new cases generator, not old one

2e8773b

Update generated files

ef7746f

Merge branch 'test-tier1-generator' into unused-to-skips

780bd87

Merge branch 'main' into unused-to-skips

89c8034

Add comments

97973db

markshannon added the skip news label Dec 15, 2023

markshannon requested a review from gvanrossum as a code owner December 15, 2023 13:55

bedevere-app bot added the awaiting core review label Dec 15, 2023

bedevere-app bot mentioned this pull request Dec 15, 2023

The code generated for the interpreter(s) is too fragile #111485

Closed

iritkatriel reviewed Dec 15, 2023

View reviewed changes

gvanrossum reviewed Dec 16, 2023

View reviewed changes

markshannon mentioned this pull request Dec 18, 2023

GH-111485: Test new cases generator #113252

Merged

iritkatriel approved these changes Dec 18, 2023

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Dec 18, 2023

markshannon merged commit 70d378c into python:main Dec 18, 2023

bedevere-app bot removed the awaiting merge label Dec 18, 2023

ryan-duve pushed a commit to ryan-duve/cpython that referenced this pull request Dec 26, 2023

pythonGH-111485: Break up instructions with unused cache entries into…

67e1fc2

… component micro-ops (pythonGH-113169)

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythonGH-111485: Break up instructions with unused cache entries into…

89e7a82

… component micro-ops (pythonGH-113169)

markshannon deleted the unused-to-skips branch August 6, 2024 10:18

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythonGH-111485: Break up instructions with unused cache entries into…

d166205

… component micro-ops (pythonGH-113169)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

GH-111485: Break up instructions with unused cache entries into component micro-ops#113169

GH-111485: Break up instructions with unused cache entries into component micro-ops#113169
markshannon merged 8 commits intopython:mainfrom
faster-cpython:unused-to-skips

markshannon commented Dec 15, 2023 •

edited

Loading

Uh oh!

iritkatriel Dec 15, 2023

Uh oh!

markshannon Dec 15, 2023

Uh oh!

gvanrossum Dec 16, 2023

Uh oh!

iritkatriel Dec 15, 2023

Uh oh!

markshannon Dec 15, 2023

Uh oh!

gvanrossum left a comment

Uh oh!

gvanrossum Dec 16, 2023

Uh oh!

gvanrossum Dec 16, 2023

Uh oh!

gvanrossum Dec 16, 2023

Uh oh!

markshannon Dec 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Uh oh!

Conversation

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

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!

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

markshannon commented Dec 15, 2023 •

edited

Loading