Bug 877878 introduces branch instrumenting feature to the Baseline compiler. Based on these profiling data we are able to try some new optimizations in IonMonkey (Bug 877872). One possible transformation is to make blocks tight by reordering them, moving unused blocks to the end of block lists. Since several MIR optimizations assert MBasicBlocks are organized in PRO order, swapping MBasicBlocks may cause these algorithms crash. Instead of manipulating MIR blocks we are trying to reorder LIR blocks between Register Allocation and Code Generation passes first, to see how much performance benefits we might gain.

For early review. This patch might cause assertion failures and SIGSEGV. I haven't figured it out currently.

Comment on attachment 785767 [details] [diff] [review] Reorder LBlocks before codegen Review of attachment 785767 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/Ion.cpp @@ +1276,5 @@ > } > } else { > + if (js_IonOptions.baselineBranchProfiling) { > + if (!lir->reorderBlocks()) { > + IonSpew(IonSpew_BranchProfiles, "lir->reorderBlocks() FAIL!!"); if the reorder is not working, then we return false. like we do for other optimization phases. And we spew the LIR graph after. @@ +1285,1 @@ > if (!codegen->generate()) { nit: Add a new line between these if statements. ::: js/src/ion/Ion.h @@ +218,5 @@ > inlineMaxTotalBytecodeLength(1000), > inlineUseCountRatio(128), > eagerCompilation(false), > usesBeforeCompilePar(1), > + baselineBranchProfiling(true) only if you bring improvements on benchmarks. ::: js/src/ion/LIR.cpp @@ +50,5 @@ > blocks_.erase(blocks_.begin() + i); > } > > +bool > +LIRGraph::reorderBlocks() rename to moveUnlikelyBlocks. @@ +63,5 @@ > +#endif > + > + // Do not pop the first block (entry block). > + while (blocks_.length() > 1) { > + LBlock *lblock = blocks_.popCopy(); popCopy will return the last block first. I think we want to iterate linearly, and clear the buffer before doing some appendAll. @@ +67,5 @@ > + LBlock *lblock = blocks_.popCopy(); > + JS_ASSERT(lblock); > + MBasicBlock *mblock = lblock->mir(); > + JS_ASSERT(mblock); > + if (mblock->isBlockUseCountAvailable() && mblock->getBlockUseCount() == 0) { I think we want to extract this condition and place it in a separated function which would be call something like "unlikelyBlockHeurisitic", and rename "unused" by "unlikely". I think one of the logic to add to the unlikely branch heuristics would be, for example, to check if the branch has less than 1% hits compared to the average of the successors of its predecessor and otherwise if its predecessors has only one successor, then inherit the likelyhood from the predecessor. @@ +82,5 @@ > + while (usedBlocks.length()) { > + if (!blocks_.append(usedBlocks.popCopy())) > + return false; > + } > + //if (blocks_.appendAll(usedBlocks)) if you replace the previous while loop by something which iterates linearly, then you can do: blocks_.clear(); DebugOnly<bool> ok; ok = blocks_.appendAll(likelyBlocks); JS_ASSERT(ok); ok = blocks_.appendAll(unlikelyBlocks); JS_ASSERT(ok);

Fixed nits & function names mentioned above. The patch crashes some regression tests, which I still could not figure out now. One of them is jit-test/tests/v8-v5/check-deltablue.js. The error message is: jit-test/tests/v8-v5/check-deltablue.js:745:18 TypeError: cc is undefined The related js code: > Planner.prototype.addConstraintsConsumingTo = function (v, coll) { > var determining = v.determinedBy; > var cc = v.constraints; > for (var i = 0; i < cc.size(); i++) { > var c = cc.at(i); > if (c != determining && c.isSatisfied()) > coll.add(c); > } > } It seems like that the offset of the property 'v.constrants' was pre-calcuated before LBlock reorder phase, assuming the offset of each LBlock would not change.

Comment on attachment 787795 [details] [diff] [review] WIP Review of attachment 787795 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonAnalysis.cpp @@ +105,5 @@ > + LBlockVector unlikelyBlocks; > + > + // The entry block must remain in the first place. > + JS_ASSERT(lir->numBlocks()); > + JS_ASSERT(likelyBlocks.append(lir->getBlock(0))); Anything in a JS_ASSERT does not produce any code in an optimized build. So this is translated to a no-op on an optimized build. Also we want to handle any allocation error by returning false here. @@ +118,5 @@ > + // which would be call something like "unlikelyBlockHeurisitic". > + // > + // Currently use naive heuristic instead. > + if (mblock->isBlockUseCountAvailable() && mblock->getBlockUseCount() == 0) { > + JS_ASSERT(unlikelyBlocks.append(lblock)); Why JS_ASSERT ? We want to handle the error value and return false if we cannot allocate. @@ +128,5 @@ > + bool success = lir->replaceBlocksByLikelyhood(likelyBlocks, unlikelyBlocks); > + JS_ASSERT(success); > + > + // Question: should we call clear() for Vector explicitly? > + likelyBlocks.clear(); Answer: No, we do not have to call clear, they would be cleared by the destructor. ::: js/src/ion/LIR.cpp @@ +59,5 @@ > + JS_ASSERT(getBlock(0) == likelyBlocks[0]); > + > + blocks_.clear(); > + JS_ASSERT(blocks_.appendAll(likelyBlocks)); > + JS_ASSERT(blocks_.appendAll(unlikelyBlocks)); JS_ASSERT is only encoded in debug builds. Which means that during an optimizing build you will append none of the block, which is why I recommended the DebugOnly class. We use JS_ASSERT because we know that the array of blocks_ should have the correct size and that we would not need to do any reallocation. @@ +63,5 @@ > + JS_ASSERT(blocks_.appendAll(unlikelyBlocks)); > + > + JS_ASSERT(num == numBlocks()); > + > + return true; No need to return a boolean, this function must be infallible as the number of block must be equal to the original number of block.

After renumbering Mblocks this version still has bugs that make the assertions in jit-tests failed. Looks like some other things were broken after the reordering.

Comment on attachment 788049 [details] [diff] [review] WIP, with testing code Review of attachment 788049 [details] [diff] [review]: ----------------------------------------------------------------- This sounds good. I think having spew for iongraph might help you understanding what is going on, and you can then compare it with IONFLAGS=filter,codegen,logs IONFILTER=myfile:123 to understand what is going wrong. Unless you hit an assertion before. Next time, you should report the assertion messages when you hit one, such as I can get an idea on what might be wrong. ::: js/src/ion/Ion.cpp @@ +1274,5 @@ > js_delete(codegen); > return NULL; > } > } else { > + if (js_IonOptions.baselineBranchProfiling && js_IonOptions.moveUnlikelyBlocks) { Based on your comment, checking moveUnlikelyBlocks implies baselineBranchProfiling, so there is no need to check both ;) @@ +1278,5 @@ > + if (js_IonOptions.baselineBranchProfiling && js_IonOptions.moveUnlikelyBlocks) { > + if (!MoveUnlikelyBlocks(lir)) { > + js_delete(codegen); > + return NULL; > + } You should add some spew which can be read with iongraph here. This would help probably help you to figure out what is going wrong with the function which have a different behavior. ::: js/src/ion/LIR.cpp @@ +53,5 @@ > +void > +LIRGraph::replaceBlocksByLikelyhood(LBlockVector &likelyBlocks, LBlockVector &unlikelyBlocks) > +{ > + // TODO: replace with mozilla::DebugOnly<size_t> num = numBlocks(); > + // size_t num = numBlocks(); you can remove this comment, as you updated the code accordingly. @@ +88,5 @@ > + return true; > +} > + > +void > +LIRGraph::dumpLastInstructions() you should not need this function if you add the spew of the graph and use iongraph.

It turns out that not only entry block should be untouched, but also the last block should remain in the original place. CodeGenerator::visitReturn(LReturn *lir) doesn't generate a jump when the LBlock which includes the LReturn instruction is on the last position in LIRGraph's block list. We don't want to touch CodeGenerator, so the last LBlock must remain in the last place, as well as the entry block. Now only three regression tests are failed: FAILURES: m-c/js/src/jit-test/tests/collections/Map-scale.js m-c/js/src/jit-test/tests/collections/Set-scale.js m-c/js/src/jit-test/tests/ion/testPos.js Based on the backtraces, I suspect that one bug caused all three failures. The information that IonSpewer/IonGraph dumped were incomplete, since the bug were hit at JIT compiling time. I examined the output of IonGraph and found nothing obviously wrong. m-c/js/src/jit-test/tests/collections/Map-scale.js (gdb) r Assertion failure: !empty(), at ./dist/include/mozilla/Vector.h:1033 Program received signal SIGSEGV, Segmentation fault. 0x0000000000ba95c2 in mozilla::VectorBase<unsigned int, 0ul, js::SystemAllocPolicy, js::Vector<unsigned int, 0ul, js::SystemAllocPolicy> >::popBack (this=0x1bb9428) at ./dist/include/mozilla/Vector.h:1033 1033 MOZ_ASSERT(!empty()); (gdb) bt #0 0x0000000000ba95c2 in mozilla::VectorBase<unsigned int, 0ul, js::SystemAllocPolicy, js::Vector<unsigned int, 0ul, js::SystemAllocPolicy> >::popBack (this=0x1bb9428) at ./dist/include/mozilla/Vector.h:1033 #1 0x0000000000ba79c8 in js::ion::CodeGeneratorShared::dropArguments (this=0x1bb8af0, argc=3) at m-c/js/src/jit/shared/CodeGenerator-shared.cpp:554 #2 0x00000000008e7ab0 in js::ion::CodeGenerator::visitCallNative (this=0x1bb8af0, call=0x1c7c7b0) at m-c/js/src/jit/CodeGenerator.cpp:1526 #3 0x00000000009d11f5 in js::ion::LCallNative::accept (this=0x1c7c7b0, visitor=0x1bb8af0) at m-c/js/src/jit/LIR-Common.h:1021 #4 0x00000000008ebd25 in js::ion::CodeGenerator::generateBody (this=0x1bb8af0) at m-c/js/src/jit/CodeGenerator.cpp:2655 #5 0x00000000008f7de7 in js::ion::CodeGenerator::generate (this=0x1bb8af0) at m-c/js/src/jit/CodeGenerator.cpp:5443 #6 0x000000000091d820 in js::ion::GenerateCode (mir=0x1bb9bd0, lir=0x1c79408, maybeMasm=0x0) at m-c/js/src/jit/Ion.cpp:1294 #7 0x000000000091d89f in js::ion::CompileBackEnd (mir=0x1bb9bd0, maybeMasm=0x0) at m-c/js/src/jit/Ion.cpp:1313 #8 0x000000000091defb in js::ion::IonCompile (cx=0x1b992f0, script=0x7ffff6255280, baselineFrame=0x7fffffffbf18, osrPc=0x1bae228 "\343\001\232", constructing=false, executionMode= js::ion::SequentialExecution) at m-c/js/src/jit/Ion.cpp:1485 #9 0x000000000091e72e in js::ion::Compile (cx=0x1b992f0, script=0x7ffff6255280, osrFrame=0x7fffffffbf18, osrPc=0x1bae228 "\343\001\232", constructing=false, executionMode=js::ion::SequentialExecution) at m-c/js/src/jit/Ion.cpp:1643 #10 0x000000000091e93e in js::ion::CanEnterAtBranch (cx=0x1b992f0, script=0x7ffff6255280, osrFrame=0x7fffffffbf18, pc=0x1bae228 "\343\001\232", isConstructing=false) at m-c/js/src/jit/Ion.cpp:1687 #11 0x00000000008959f5 in js::ion::EnsureCanEnterIon (cx=0x1b992f0, stub=0x1babda0, frame=0x7fffffffbf18, script=0x7ffff6255280, pc=0x1bae228 "\343\001\232", jitcodePtr=0x7fffffffbe60) at m-c/js/src/jit/BaselineIC.cpp:703 #12 0x00000000008961a0 in js::ion::DoUseCountFallback (cx=0x1b992f0, stub=0x1babda0, frame=0x7fffffffbf18, infoPtr=0x7fffffffbef0) at m-c/js/src/jit/BaselineIC.cpp:889 #13 0x00007ffff7ff1f59 in ?? () #14 0x00007ffff7ff277d in ?? () #15 0x00007fffffffbeb8 in ?? () #16 0x0000000000000002 in ?? () #17 0x0000000001b70560 in js::ion::R2 () #18 0x00007ffff633f370 in ?? () #19 0x00007ffff7ff2011 in ?? () #20 0x0000000000000302 in ?? () #21 0x0000000001babda0 in ?? () #22 0x00007fffffffbf18 in ?? () #23 0x00007fffffffbef0 in ?? () #24 0x0000000000000000 in ?? () (gdb) m-c/js/src/jit-test/tests/collections/Set-scale.js (gdb) r Assertion failure: !empty(), at ./dist/include/mozilla/Vector.h:1033 Program received signal SIGSEGV, Segmentation fault. 0x0000000000ba95c2 in mozilla::VectorBase<unsigned int, 0ul, js::SystemAllocPolicy, js::Vector<unsigned int, 0ul, js::SystemAllocPolicy> >::popBack (this=0x1bb1498) at ./dist/include/mozilla/Vector.h:1033 1033 MOZ_ASSERT(!empty()); (gdb) bt #0 0x0000000000ba95c2 in mozilla::VectorBase<unsigned int, 0ul, js::SystemAllocPolicy, js::Vector<unsigned int, 0ul, js::SystemAllocPolicy> >::popBack (this=0x1bb1498) at ./dist/include/mozilla/Vector.h:1033 #1 0x0000000000ba79c8 in js::ion::CodeGeneratorShared::dropArguments (this=0x1bb0b60, argc=3) at m-c/js/src/jit/shared/CodeGenerator-shared.cpp:554 #2 0x00000000008e7ab0 in js::ion::CodeGenerator::visitCallNative (this=0x1bb0b60, call=0x1c796f8) at m-c/js/src/jit/CodeGenerator.cpp:1526 #3 0x00000000009d11f5 in js::ion::LCallNative::accept (this=0x1c796f8, visitor=0x1bb0b60) at m-c/js/src/jit/LIR-Common.h:1021 #4 0x00000000008ebd25 in js::ion::CodeGenerator::generateBody (this=0x1bb0b60) at m-c/js/src/jit/CodeGenerator.cpp:2655 #5 0x00000000008f7de7 in js::ion::CodeGenerator::generate (this=0x1bb0b60) at m-c/js/src/jit/CodeGenerator.cpp:5443 #6 0x000000000091d820 in js::ion::GenerateCode (mir=0x1bafc60, lir=0x1c764b0, maybeMasm=0x0) at m-c/js/src/jit/Ion.cpp:1294 #7 0x000000000091d89f in js::ion::CompileBackEnd (mir=0x1bafc60, maybeMasm=0x0) at m-c/js/src/jit/Ion.cpp:1313 #8 0x000000000091defb in js::ion::IonCompile (cx=0x1b992f0, script=0x7ffff6255280, baselineFrame=0x7fffffffbf18, osrPc=0x1bae222 "\343\001\232", constructing=false, executionMode= js::ion::SequentialExecution) at m-c/js/src/jit/Ion.cpp:1485 #9 0x000000000091e72e in js::ion::Compile (cx=0x1b992f0, script=0x7ffff6255280, osrFrame=0x7fffffffbf18, osrPc=0x1bae222 "\343\001\232", constructing=false, executionMode=js::ion::SequentialExecution) at m-c/js/src/jit/Ion.cpp:1643 #10 0x000000000091e93e in js::ion::CanEnterAtBranch (cx=0x1b992f0, script=0x7ffff6255280, osrFrame=0x7fffffffbf18, pc=0x1bae222 "\343\001\232", isConstructing=false) at m-c/js/src/jit/Ion.cpp:1687 #11 0x00000000008959f5 in js::ion::EnsureCanEnterIon (cx=0x1b992f0, stub=0x1badd50, frame=0x7fffffffbf18, script=0x7ffff6255280, pc=0x1bae222 "\343\001\232", jitcodePtr=0x7fffffffbe60) at m-c/js/src/jit/BaselineIC.cpp:703 #12 0x00000000008961a0 in js::ion::DoUseCountFallback (cx=0x1b992f0, stub=0x1badd50, frame=0x7fffffffbf18, infoPtr=0x7fffffffbef0) at m-c/js/src/jit/BaselineIC.cpp:889 #13 0x00007ffff7ff1f59 in ?? () #14 0x00007ffff7ff2763 in ?? () #15 0x00007fffffffbeb8 in ?? () #16 0x0000000000000002 in ?? () #17 0x0000000001b70560 in js::ion::R2 () #18 0x00007ffff633f370 in ?? () #19 0x00007ffff7ff2011 in ?? () #20 0x0000000000000302 in ?? () #21 0x0000000001badd50 in ?? () #22 0x00007fffffffbf18 in ?? () #23 0x00007fffffffbef0 in ?? () #24 0x0000000000000000 in ?? () (gdb) m-c/js/src/jit-test/tests/ion/testPos.js (gdb) r Assertion failure: !empty(), at ./dist/include/mozilla/Vector.h:1033 Program received signal SIGSEGV, Segmentation fault. 0x0000000000ba95c2 in mozilla::VectorBase<unsigned int, 0ul, js::SystemAllocPolicy, js::Vector<unsigned int, 0ul, js::SystemAllocPolicy> >::popBack (this=0x1bb8d38) at ./dist/include/mozilla/Vector.h:1033 1033 MOZ_ASSERT(!empty()); (gdb) bt #0 0x0000000000ba95c2 in mozilla::VectorBase<unsigned int, 0ul, js::SystemAllocPolicy, js::Vector<unsigned int, 0ul, js::SystemAllocPolicy> >::popBack (this=0x1bb8d38) at ./dist/include/mozilla/Vector.h:1033 #1 0x0000000000ba79c8 in js::ion::CodeGeneratorShared::dropArguments (this=0x1bb8400, argc=3) at m-c/js/src/jit/shared/CodeGenerator-shared.cpp:554 #2 0x00000000008e7ab0 in js::ion::CodeGenerator::visitCallNative (this=0x1bb8400, call=0x1c8be78) at m-c/js/src/jit/CodeGenerator.cpp:1526 #3 0x00000000009d11f5 in js::ion::LCallNative::accept (this=0x1c8be78, visitor=0x1bb8400) at m-c/js/src/jit/LIR-Common.h:1021 #4 0x00000000008ebd25 in js::ion::CodeGenerator::generateBody (this=0x1bb8400) at m-c/js/src/jit/CodeGenerator.cpp:2655 #5 0x00000000008f7de7 in js::ion::CodeGenerator::generate (this=0x1bb8400) at m-c/js/src/jit/CodeGenerator.cpp:5443 #6 0x000000000091d820 in js::ion::GenerateCode (mir=0x1baf4f0, lir=0x1c85650, maybeMasm=0x0) at m-c/js/src/jit/Ion.cpp:1294 #7 0x000000000091d89f in js::ion::CompileBackEnd (mir=0x1baf4f0, maybeMasm=0x0) at m-c/js/src/jit/Ion.cpp:1313 #8 0x000000000091defb in js::ion::IonCompile (cx=0x1b992f0, script=0x7ffff6255280, baselineFrame=0x7fffffffbf28, osrPc=0x1baf1dd "\343\001\232", constructing=false, executionMode= js::ion::SequentialExecution) at m-c/js/src/jit/Ion.cpp:1485 #9 0x000000000091e72e in js::ion::Compile (cx=0x1b992f0, script=0x7ffff6255280, osrFrame=0x7fffffffbf28, osrPc=0x1baf1dd "\343\001\232", constructing=false, executionMode=js::ion::SequentialExecution) at m-c/js/src/jit/Ion.cpp:1643 #10 0x000000000091e93e in js::ion::CanEnterAtBranch (cx=0x1b992f0, script=0x7ffff6255280, osrFrame=0x7fffffffbf28, pc=0x1baf1dd "\343\001\232", isConstructing=false) at m-c/js/src/jit/Ion.cpp:1687 #11 0x00000000008959f5 in js::ion::EnsureCanEnterIon (cx=0x1b992f0, stub=0x1c65090, frame=0x7fffffffbf28, script=0x7ffff6255280, pc=0x1baf1dd "\343\001\232", jitcodePtr=0x7fffffffbe70) at m-c/js/src/jit/BaselineIC.cpp:703 #12 0x00000000008961a0 in js::ion::DoUseCountFallback (cx=0x1b992f0, stub=0x1c65090, frame=0x7fffffffbf28, infoPtr=0x7fffffffbf00) at m-c/js/src/jit/BaselineIC.cpp:889 #13 0x00007ffff7ff1f59 in ?? () #14 0x00007ffff7ff2f17 in ?? () #15 0x00007fffffffbec8 in ?? () #16 0x0000000000000002 in ?? () #17 0x0000000001b70560 in js::ion::R2 () #18 0x00007ffff633f370 in ?? () #19 0x00007ffff7ff2011 in ?? () #20 0x0000000000000302 in ?? () #21 0x0000000001c65090 in ?? () #22 0x00007fffffffbf28 in ?? () #23 0x00007fffffffbf00 in ?? () #24 0x0000000000000000 in ?? () (gdb)

(In reply to Wei Wu [:wuwei UTC+8] from comment #7) > It turns out that not only entry block should be untouched, but also the > last block should remain in the original place. > > CodeGenerator::visitReturn(LReturn *lir) doesn't generate a jump when the > LBlock which includes the LReturn instruction is on the last position in > LIRGraph's block list. We don't want to touch CodeGenerator, so the last > LBlock must remain in the last place, as well as the entry block. I do not see why we should not touch the CodeGenerator if it is not working with our input, it would be an easy fix to modify the logic inside visitReturn, to check that this is the last LIR block instead of the last MIR block, which can be done by checking the newly set block->id(). An other thing which might be interesting to investigate would be to generate the epilogue at the end of the likely blocks, but this should be made part of a follow up bug. The reason being that we will load extra code just for exiting the function.

Comment on attachment 789009 [details] [diff] [review] WIP, with ONE bug. Review of attachment 789009 [details] [diff] [review]: ----------------------------------------------------------------- This patch is looking mostly like the previous one. I do not see any need for an extra feedback. The feedback is mostly for design choices, or knowing if this is the right direction. Just the comment would have been enough in this case.

IonSpew dumps for m-c/js/src/jit-test/tests/collections/Map-scale.js

(In reply to Wei Wu [:wuwei UTC+8] from comment #7) > Created attachment 789009 [details] [diff] [review] > WIP, with ONE bug. > > Now only three regression tests are failed: > FAILURES: > m-c/js/src/jit-test/tests/collections/Map-scale.js > m-c/js/src/jit-test/tests/collections/Set-scale.js > m-c/js/src/jit-test/tests/ion/testPos.js > > Based on the backtraces, I suspect that one bug caused all three failures. > > The information that IonSpewer/IonGraph dumped were incomplete, since the > bug were hit at JIT compiling time. I examined the output of IonGraph and > found nothing obviously wrong. when IonMonkey compiles 'f(a, g(b, c), d)' it must push arguments 'a', 'b', and 'c' on stack and then call function 'g'. After 'g' returns both 'b' and 'c' are popped out, leaving 'a' on stack for the invocation of function 'f'. 'a' will stay on stack until 'f' returns. The problem is that this expression might be splitted into a few blocks, and we might break these data dependencies by putting the pop instruction in front of corresponding push instruction. A patch for Bug 906418 is about to fix it.

WIP. Currently it improves kraken by ~1%, but slows down Octane by ~7%.

Comment on attachment 800473 [details] [diff] [review] moveLBlocksByLikelyhood.patch Review of attachment 800473 [details] [diff] [review]: ----------------------------------------------------------------- Nice work, still I think we need more comments and assertions to ensure that at least one likely path exists. ::: js/src/jit/IonAnalysis.cpp @@ +173,5 @@ > + MBasicBlock *succ = block->getSuccessor(i); > + if (succ->isBlockUseCountAvailable()) > + sum += succ->getBlockUseCount(); > + } > + if (sum < 300 && block->isMarked()) { can you add comment above the condition such as: // We do not have enough samples to make any wise choice. // If we have less than 300 iterations, we just assume that all successors are likely. @@ +189,5 @@ > + if (succ->isMarked()) > + continue; > + if (block->info().script() != succ->info().script() || > + !succ->isBlockUseCountAvailable() || > + succ->getBlockUseCount() > 0.01 * sum / block->numSuccessors() || This 0.01 should be put in a constant, and we can look at tweaking it later. @@ +191,5 @@ > + if (block->info().script() != succ->info().script() || > + !succ->isBlockUseCountAvailable() || > + succ->getBlockUseCount() > 0.01 * sum / block->numSuccessors() || > + succ->isLoopHeader() || > + succ->isLoopBackedge()) { block->isLoopHeader(), not succ. succ->isLoopBackedge should not be needed. @@ +193,5 @@ > + succ->getBlockUseCount() > 0.01 * sum / block->numSuccessors() || > + succ->isLoopHeader() || > + succ->isLoopBackedge()) { > + succ->mark(); > + worklist.append(succ); We should assert that at least one successor is marked. Otherwise this would be a dead-end. @@ +196,5 @@ > + succ->mark(); > + worklist.append(succ); > + } else { > + for (size_t i = 0; i < block->numPredecessors(); i++) { > + if (block->loopDepth() < block->getPredecessor(i)->loopDepth() && I do not understand this for loop. I think the condition should be: block->loopDepth() > succ->loopDepth() => succ->mark() @@ +197,5 @@ > + worklist.append(succ); > + } else { > + for (size_t i = 0; i < block->numPredecessors(); i++) { > + if (block->loopDepth() < block->getPredecessor(i)->loopDepth() && > + block->immediateDominator()->isMarked()) { if block is marked, then its immediateDominator must be marked. In fact I think you should assert that. Otherwise, this would mean that a likely code can be located under an unlikely code.

Fixed some nits. More comments and assertions have been added.

After the landing of bug 1209515, bug 877878 and this bug became unnecessary. wontfix. This optimization was based on bug 877878, and sadly failed to gain performance benefits, hence never got landed.