Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
GH-105229: Replace some superinstructions with single instruction equivalent.#105230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure if these need a name, but suggestions for a better name are welcome.
"Combined instructions", maybe?
| def_op('DICT_UPDATE',165) | ||
| def_op('LOAD_FAST_LOAD_FAST',168) | ||
| def_op('STORE_FAST_LOAD_FAST',169) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just curious, how are the stats on this? Is it worth keeping?
I would expect most of these pairs to span multiple lines. Although maybe comprehensions and generator expressions make up for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't know, as you say it might be quite low.
I think we should keep it for this PR anyway, as we are replacing two-codeunit superinstructions with the one-codeunit equivalent. We can change the choice of superinstructions in another PR.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -0,0 +1 @@ | |||
| Replace some dynamic superinstructions will single instruction equivalents. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| Replace some dynamic superinstructionswill single instruction equivalents. | |
| Replace some dynamic superinstructionswith single instruction equivalents. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| staticvoid | ||
| make_super_instruction(cfg_instr*inst1,cfg_instr*inst2,intsuper_op) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| make_super_instruction(cfg_instr*inst1,cfg_instr*inst2,intsuper_op) | |
| make_combined_instruction(cfg_instr*inst1,cfg_instr*inst2,intcombined_op) |
| @@ -0,0 +1 @@ | |||
| Replace some dynamic superinstructions will single instruction equivalents. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| Replace some dynamic superinstructionswill single instruction equivalents. | |
| Replace some dynamic superinstructionswith single instruction equivalents. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Heh. Crossed posts.
| /* Skip if instructions are on different lines */ | ||
| if (inst1->i_loc.lineno!=inst2->i_loc.lineno) { | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I saw a comment from@brandtbucher that this requirement eliminated most super-instructions (faster-cpython/ideas#585 (comment)). And yet your pyperformance results on this PR are quite good. I wonder how these two things are both true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For instructions without observable side effects (e.g.LOAD_FAST, or maybePOP_TOP), can't we still do a super instruction and keep theNOP for the extra lineno?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I saw a comment
Oh, I realize now that this comment was specific toSTORE_FAST__LOAD_FAST.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wonder how these two things are both true.
Oh, I guess the most likely answer is infaster-cpython/ideas#585 (comment) :
it appears that the reduction in code size more than compensates.
markshannonJun 5, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That is just my hypothesis.
The goal is to remove superinstructions that cross block and line boundaries, as they are problematic.
I was just trying to not slow things down by removing them. A speedup was just a pleasant surprise.
Even if the speedup is mainly from layout changes, or cache effects, the main motivation of this PR remains valid.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Shows a surprising, but pleasing,1.7% speedup.
These are common instructions, so the reduction in code size and memory reads is considerable.
Infaster-cpython/ideas#585 I refer these to as "single instruction superinstructions", which is a rather clumsy name. I'm not sure if these need a name, but suggestions for a better name are welcome.
I've left the superinstructions involving
LOAD_CONSTalone, as we are looking at other approaches to speed up constants.