gh-109039: Branch prediction for Tier 2 interpreter by gvanrossum · Pull Request #109038 · python/cpython

gvanrossum · 2023-09-06T23:42:19Z

Add cache entries to bytecodes.c and update them (but don't use them yet)
Make tests pass
Use cache entries for branch prediction
Add new tests
Initialize cache entries to 0x5555 (0b_0101_0101_0101_0101)
Buildbots
Benchmark

Issue: Branch prediction design for Tier 2 (uops) interpreter #109039

iritkatriel · 2023-09-07T10:19:02Z

+ #if ENABLE_SPECIALIZATION
+ next_instr->cache = (next_instr->cache << 1) | flag;
+ #endif
+ JUMPBY(oparg * flag);


Don't you need also a SKIP_OVER the cache?
I'm guessing that could cause the assert(frame->prev_instr == instr); in _Py_call_instrumentation_jump to fail for the instrumented jumps.

That skip over the cache is already generated -- see the corresponding code in generated_cases.c.h.

You mean the "next_instr += 1;"? In all other cases there is an explicit SKIP_OVER(INLINE_CACHE_ENTRIES_...) in bytecodes.c.

Those SKIP_OVER() calls are always followed by a DISPATCH() call (or maybe a goto).

I see. Can we make the code generator emit SKIP_OVER(X) instead of next_instr += x;?

We can, though IIRC Mark at some point objected to emitting macros. So I'd rather keep the status quo.

@markshannon What is the reason not to emit macros?
A reason to emit them is so that they are implemented in one place, so if their implementation changes you only change there. Do we want to change the code generator (and to remember that we need to) every time the implementation of a macro like SKIP_OVER changes?

Honestly I don't expect SKIP_OVER() to ever change. In hand-written code the macro expresses the intent better. But in generated code it just obscures what happens. I had to go to some lengths to change PEEK() and POKE() calls in the generated code to using stack_pointer[x] instead; I don't want to go back. If you still disagree, try engaging @markshannon.

If you still disagree,
More like trying to understand than disagreeing.
try engaging @markshannon.
Yes I directed my previous comment to him.

This is needed so branch prediction can work.

Alas, this goes untested (how to test it?). In INSTRUMENTED_POP_JUMP_IF_NOT_NONE, rename flag to nflag to emphasise that its sense is reversed (this is the only op that jumps if the flag is false, because there's no Py_IsNotNone() function). (Alternatively, we could have changed the sense of the flag, but that would have been more work.)

gvanrossum · 2023-09-08T00:45:43Z

Benchmark is running, will post results here. I think I've addressed all actionable review comments. Please review.

gvanrossum · 2023-09-08T03:54:41Z

Benchmark is neutral(*), which is a good thing (it means adding a cache entry to the branch instructions didn't slow anything down).

(*) Or possibly some benchmarks crashed. I'm going to run buildbots to be sure.

bedevere-bot · 2023-09-08T04:45:55Z

🤖 New build scheduled with the buildbot fleet by @gvanrossum for commit 1850988 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

iritkatriel · 2023-09-08T06:14:13Z

+ family_member_names.update(family.members)
+ for instr in self.instrs.values():
+ if (
+ instr.name not in family_member_names


Why do we need to exclude family members from this table?

That's tradition -- the table only contains the data for family heads and is always consulted after looking up the deoptimized opcode in _PyOpcode_Deopt.

exclude family members from this table?
That's tradition
At first glance I thought this was some reference to a hypothetical family in the midst of conflict. 🙂

gvanrossum · 2023-09-08T15:57:58Z

Hm. Many buildbots fail on test_sys_settrace, but I can't (yet) reproduce it. Must be about build flags.

This time by disabling the optimizer.

AlexWaygood · 2023-09-08T16:24:44Z

Hm. Many buildbots fail on test_sys_settrace, but I can't (yet) reproduce it. Must be about build flags.

Unlikely that it's to do with this PR, as it's already happening on main -- see #109052 and #109143

gvanrossum · 2023-09-08T16:36:34Z

Unlikely that it's to do with this PR, as it's already happening on main -- see #109052 and #109143

Thanks, I'll not worry about it then.

Buildbots are beginning to lose their value for me. :-(

gvanrossum · 2023-09-11T17:31:29Z

I'll fix the conflict, then merge this.

gvanrossum · 2023-09-11T17:56:03Z

(Sorry, several reviewers got a review request because I changed the bytecode magic number. Please ignore.)

markshannon · 2023-10-30T10:58:18Z

This introduced a regression in branch and jump monitoring, as the target is off by one.
The line numbers in test_monitoring should be unchanged from 3.12.

gvanrossum · 2023-10-30T18:04:09Z

This introduced a regression in branch and jump monitoring, as the target is off by one. The line numbers in test_monitoring should be unchanged from 3.12.

Okay, can you give me a hint on what went wrong? I haven't been following how the instrumentation works in detail, and I have no idea which bits are being tested by the tests I modified, or what I should fix. (If it's involved, please open a new issue and CC me.)

markshannon · 2023-10-31T09:35:23Z

It is fixed in #111486, so nothing to worry about. Just putting it here for the record.

gvanrossum added 2 commits September 6, 2023 16:35

inst() and macro() may need cache size metadata
44db701

Add cache entry to *POP_JUMP_IF_* instructions
ff29ab3

gvanrossum changed the title ~~Branch prediction for Tier 2 interpreter~~gh-109039: Branch prediction for Tier 2 interpreterSep 6, 2023

bedevere-bot mentioned this pull request Sep 6, 2023
Branch prediction design for Tier 2 (uops) interpreter #109039
Closed

gvanrossum added the skip news label Sep 6, 2023

gvanrossum added 2 commits September 6, 2023 17:40

Fix test_dis (also fixes test_peepholer)
ebc91a2

Fix test_monitoring
072bb38

gvanrossum mentioned this pull request Sep 7, 2023
Stitching it all together faster-cpython/ideas#621
Open

Include pycore_bitutils.h in instrumentation.c
896ae53

iritkatriel reviewed Sep 7, 2023
View reviewed changes

Comment threadTools/cases_generator/generate_cases.py

iritkatriel reviewed Sep 7, 2023
View reviewed changes

markshannon reviewed Sep 7, 2023
View reviewed changes

Comment threadPython/bytecodes.c

gvanrossum added 8 commits September 7, 2023 10:14

Follow likely jumps in trace
0eb5b90

Merge remote-tracking branch 'upstream/main' into count-branches
25bfb3d

Require 16 iterations before optimizing
a9c0805
This is needed so branch prediction can work.

Fix existing uops tests
7dfb94c

Add test for branch prediction
73eb60f

Initialize POP_JUMP_IF* counters to 0x5555
fbd322a

Simplify writing of _PyOpcode_Caches
1850988

gvanrossum marked this pull request as ready for review September 8, 2023 00:44

bedevere-bot added the awaiting core review label Sep 8, 2023

gvanrossum added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 8, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 8, 2023

iritkatriel reviewed Sep 8, 2023
View reviewed changes

Fix test_huntrleaks under -Xuops
4f1684c
This time by disabling the optimizer.

Fix test_dis under -Xuops
cb2cf12

Merge branch 'main' into count-branches
d74670c

gvanrossum enabled auto-merge (squash) September 11, 2023 17:36

gvanrossum disabled auto-merge September 11, 2023 17:45

Update magic number
41463a5

gvanrossum requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners September 11, 2023 17:54

gvanrossum enabled auto-merge (squash) September 11, 2023 17:54

gvanrossum removed request for brettcannon, ericsnowcurrently, ncoghlan and warsaw September 11, 2023 17:55

gvanrossum merged commit bcce5e2 into python:mainSep 11, 2023

bedevere-appBot removed the awaiting core review label Sep 11, 2023

gvanrossum deleted the count-branches branch September 11, 2023 18:21

markshannon mentioned this pull request Sep 12, 2023
Adds stats for the tier 2 optimizer #109329
Closed

markshannon mentioned this pull request Oct 30, 2023
GH-111485: Increment next_instr consistently at the start of the instruction. #111486
Merged

Uh oh!

Uh oh!

Conversation

gvanrossum commented Sep 6, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Uh oh!

gvanrossum commented Sep 8, 2023

Uh oh!

gvanrossum commented Sep 8, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Sep 8, 2023

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 commented Sep 8, 2023

Uh oh!

AlexWaygood commented Sep 8, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Sep 8, 2023

Uh oh!

gvanrossum commented Sep 11, 2023

Uh oh!

gvanrossum commented Sep 11, 2023

Uh oh!

markshannon commented Oct 30, 2023

Uh oh!

gvanrossum commented Oct 30, 2023

Uh oh!

markshannon commented Oct 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gvanrossum commented Sep 6, 2023•
edited
Loading

gvanrossum commented Sep 8, 2023•
edited
Loading

AlexWaygood commented Sep 8, 2023•
edited
Loading