Bug 19457 – Create fused opcodes for tests and conditional jumps

• Status: RESOLVED
• Resolution: WONTFIX
• Component: JavaScriptCore

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Darin Adler]

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

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

Comment on attachment 21613 [details]
Revised proposed patch

Clearing review flag.

[Alexey Proskuryakov]

Is this bug still actionable?

[Gavin Barraclough]

Further bytecode superinstructions is not something we're likely to do right now.