Bug 811349 added hit counts for basic blocks, and text information related to those blocks for printing the LIR instructions and code generated for them. It would be good to make this more detailed, and for register allocation the most important information here is how much spill code was generated for compiled blocks, which gives a good indication of the quality of the code that was generated. (Register allocations can also differ from each other in when they use in memory vs. register operands for instructions, but that is hard to measure and should I guess be correlated with spill code generated.) The attached patch adds counts for these to the basic block information printed with -D and passed to addons in the PC count JSON. When hit counts and are combined with instruction and spill byte counts, we get a near exact estimate of the total amount of Ion instruction and spill code that has executed. Using these with an x86 debug shell build on the v8 benchmarks, I get the following fractions of spill / total code executed: crypto: 36.0% deltablue: 14.0% earley-boyer: 12.0% navier-stokes: 25.6% raytrace: 15.8% regexp: 5.2% richards: 9.4% splay: 9.2% Clearly spill code has the biggest impact on crypto and navier-stokes, but all the other tests are impacted to some degree.

Comment on attachment 684485 [details] [diff] [review] patch Review of attachment 684485 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/CodeGenerator.cpp @@ +1461,5 @@ > } > + uint32 instructionBytes = 0; > + uint32 spillBytes = 0; > + uint32 lastLength = masm.size(); > + LInstruction *last = NULL; Can you wrap all these additions (and previous one) under a class named ProfileCodeGen, such as we can keep the meaning of generateBody clean and closer to most of its content. class ProfileCodeGen { IonScriptCounts *counts; uint32 instructionBytes; uint32 spillBytes; uint32 lastLength; LInstruction *last; public: ProfilerCodeGen(IonMacroAssembler &masm); void enterBlock(size_t index); void instrumentPreInstruction(IonMacroAssembler &masm, LInstruction *ins); void instrumentPostInstruction(IonMacroAssembler &masm, LInstruction *ins); void leaveBlock(); };

Revised per comments. This also fixes a bug where the instruction bytes for the last instruction in a block were not being counted. This caused us to overestimate the spill code percentages in comment 0 by 1% or so.

Comment on attachment 684690 [details] [diff] [review] patch Review of attachment 684690 [details] [diff] [review]: ----------------------------------------------------------------- This looks better. Still one error in the profiling code, and I would prefer if the profiling class could avoid saving pointers to previous LInstruction. ::: js/src/ion/CodeGenerator.cpp @@ +1445,5 @@ > + > + uint32 instructionBytes = 0; > + uint32 spillBytes = 0; > + uint32 lastLength = masm.size(); > + LInstruction *last = NULL; Instead of a LInstruction, take a uint32 * to the last instruction counter and increment it if it is not NULL. @@ +1465,5 @@ > + // included in either the text for the block or the instruction byte > + // counts. > + masm.inc64(AbsoluteAddress(block.addressOfHitCount())); > + > + masm.setPrinter(&printer); nit: Add a comment on top of this line: // Collect a human readable version of the generated assembler to be // displayed through the PCCount API. @@ +1471,5 @@ > + } > + > + void visitInstruction(LInstruction *ins) > + { > + if (ins) nit: remove if, ins should never be NULL. @@ +1472,5 @@ > + > + void visitInstruction(LInstruction *ins) > + { > + if (ins) > + printer.printf("[%s]\n", ins->opName()); nit: Add comment on top of this line: // Prefix stream of assembly instructions with their LIR instruction name. @@ +1475,5 @@ > + if (ins) > + printer.printf("[%s]\n", ins->opName()); > + if (last) { > + uint32 &byteCount = last->isMoveGroup() ? spillBytes : instructionBytes; > + byteCount += masm.size() - lastLength; Duplicate this increment in the destructor to handle the last instruction of the block.

Backed out for build bustage on Linux/Windows/Android. Sweet, twice in one day! https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5036ddebbb https://tbpl.mozilla.org/php/getParsedLog.php?id=17314045&tree=Mozilla-Inbound e:/builds/moz2_slave/m-in-w32/build/js/src/ion/CodeGenerator.cpp(1446) : error C2864: 'js::ion::ScriptCountBlockState::instructionBytes' : only static const integral data members can be initialized within a class e:/builds/moz2_slave/m-in-w32/build/js/src/ion/CodeGenerator.cpp(1447) : error C2864: 'js::ion::ScriptCountBlockState::spillBytes' : only static const integral data members can be initialized within a class e:/builds/moz2_slave/m-in-w32/build/js/src/ion/CodeGenerator.cpp(1451) : error C2864: 'js::ion::ScriptCountBlockState::last' : only static const integral data members can be initialized within a class e:/builds/moz2_slave/m-in-w32/build/js/src/ion/CodeGenerator.cpp(1452) : error C2327: 'js::ion::ScriptCountBlockState::masm' : is not a type name, static, or enumerator e:/builds/moz2_slave/m-in-w32/build/js/src/ion/CodeGenerator.cpp(1452) : error C2065: 'masm' : undeclared identifier e:/builds/moz2_slave/m-in-w32/build/js/src/ion/CodeGenerator.cpp(1452) : error C2228: left of '.size' must have class/struct/union e:/builds/moz2_slave/m-in-w32/build/js/src/ion/CodeGenerator.cpp(1452) : error C2864: 'js::ion::ScriptCountBlockState::lastLength' : only static const integral data members can be initialized within a class

Ugh, some initialization code in the class definition due to bad copy/paste. It's pretty insane that clang accepted that code at all, no warning.