Bug 19457 - Create fused opcodes for tests and conditional jumps
Summary: Create fused opcodes for tests and conditional jumps
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-06-09 20:49 PDT by Cameron Zwarich (cpst)
Modified: 2012-09-07 00:31 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch (33.92 KB, patch)
2008-06-09 20:57 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised proposed patch (34.86 KB, patch)
2008-06-10 17:28 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-06-09 20:49:10 PDT
We have been talking about creating superinstructions for a while now. I added some instrumentation to the VM (enable DUMP_OPCODE_STATS in Opcode.h in a debug build to use it) in order to see which pairs of opcodes are most common. The most common pairs are of the form (<test>, <conditional jump>), so I figured that would be a good place to start.

The most common of these is (less, jtrue), which is 6.41% of all instruction pairs on SunSpider. I made a patch that adds a new jless instruction, which gives a 3.6% improvement on SunSpider.
Comment 1 Cameron Zwarich (cpst) 2008-06-09 20:57:39 PDT
Created attachment 21598 [details]
Proposed patch

Here's the patch. One issue I see is that CodeGenerator::lastOpcodeID is uninitialized. I wasn't sure of the best way to do it, because OpcodeID is an enum. Any suggestions? The documentation for j_less is also wrong.
Comment 2 Cameron Zwarich (cpst) 2008-06-10 17:28:23 PDT
Created attachment 21613 [details]
Revised proposed patch

Maciej says that this method of doing peephole optimizations won't conflict with his plans for fusing loads into other operations, so here's a cleaned up version of the patch for review. I initialize m_lastOpcodeID to op_end, since no optimization will ever optimize a sequence starting with op_end.

We still might want to consider a different method of code generation that incorporates all of these optimizations, but we might as well test them out incrementally before we go rewriting everything.
Comment 3 Darin Adler 2008-06-11 11:57:40 PDT
Comment on attachment 21613 [details]
Revised proposed patch

 395 void CodeGenerator::retrieveLastBinaryOp(int& dstIndex, int& src1Index, int& src2Index)
 396 {
 397     size_t size = instructions().size();
 398     dstIndex = instructions().at(size - 3).u.operand;
 399     src1Index = instructions().at(size - 2).u.operand;
 400     src2Index = instructions().at(size - 1).u.operand;
 401 }

This should assert that size is >= 4.

 405     instructions().shrink(instructions().size() - 4);

 403 void CodeGenerator::rewindBinaryOp()
 404 {
 405     instructions().shrink(instructions().size() - 4);
 406 }

This too.

r=me
Comment 4 Cameron Zwarich (cpst) 2008-06-11 12:52:09 PDT
Landed in r34497. I will keep this bug open to discuss other possibilities for test / jump opcode fusion, but this was definitely the most common case.
Comment 5 Cameron Zwarich (cpst) 2008-06-24 14:38:09 PDT
Comment on attachment 21613 [details]
Revised proposed patch

Clearing review flag.
Comment 6 Alexey Proskuryakov 2010-06-11 17:41:37 PDT
Is this bug still actionable?
Comment 7 Gavin Barraclough 2012-09-07 00:31:16 PDT
Further bytecode superinstructions is not something we're likely to do right now.