IonBuilder does way too many things currently: - cfg - analyzeTypes - jsbytecode to MIR translation Only the analyzeTypes need to happen on the mainthread. But since this is combined, we have to keep the full IonBuilder on mainthread. This bug is about splitting the cfg from the rest. The benefits of this are: - Caching the cfg: the cfg is always the same upon every build. No need to do it every time. - Easier understanding of IonBuilder. - Will make it possible to split analyzeTypes and jsbytecode to MIR translation. Next step.

Passes jit-tests on an older revision. Needs a lot of cleanup, but works. I'll probably take the time next week to get patches for my P1 bugs. But the week after that I should be able to clean this and request reviews.

Comment on attachment 8801068 [details] [diff] [review] WIP Review of attachment 8801068 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/JitOptions.cpp @@ +94,4 @@ > SET_DEFAULT(disableGvn, false); > > // Toggles whether inlining is globally disabled. > + SET_DEFAULT(disableInlining, true); // TODO enable again. Okay. Seems this WIP is not complete yet.

Comment on attachment 8801068 [details] [diff] [review] WIP Review of attachment 8801068 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/ion/ArrayLengthGetPropertyIC.js @@ -22,5 @@ > assertEq(intLength(denseArray, 10), 10); > assertEq(intLength(typedArray, 10), 10); > -// assertEq(intLength(fakeArray1, 10), 10); > - > -assertEq(valueLength(denseArray, 10), 10); I guess you are going to revert all the test cases back to their original versions after this patch (set) is complete? ::: js/src/jit/IonControlFlow.cpp @@ +42,5 @@ > +void > +ControlFlowGraph::dump(JSScript* script) > +{ > + if (blocks_.length() == 0) { > + fprintf(stderr, "Didn't run yet.\n"); nit: Use the GenericPrinter, this way we could dump it in either a file or a log if we need it. This is really cool to have that. @@ +1025,5 @@ > + // condswitch [length +exit_pc; first case offset +next-case ] > + // { > + // { > + // ... any code ... > + // case (+jump) [pcdelta offset +next-case] Thanks for improving all the comments that we were lacking in IonBuilder :) @@ +1578,5 @@ > + break; > + } > + } > + } > + nit: Strip all trailing whitespace before the final patch ;) ::: js/src/jit/IonControlFlow.h @@ +17,5 @@ > + > +namespace js { > +namespace jit { > + > +class HControlInstruction; Why the H prefix, and not CFG? @@ +297,5 @@ > + * HTest > + * > + * POP / PEEK (depending on keepCondition_) > + * IFEQ JUMP succ1 > + * IFNEQ JUMP succ2 Nice! @@ +594,5 @@ > + return blocks_.length(); > + } > +}; > + > +class ControlFlowGenerator Shouldn't this be a TempObject? @@ +602,5 @@ > + public: > + JSScript* script; > + HBlock* current; > + jsbytecode* pc; > + GSNCache gsn; Whoa, that means we no longer need the SourceNote reader within IonBuilder, nice! ::: js/src/jit/RangeAnalysis.cpp @@ +3486,5 @@ > if (!cond->isTest()) > continue; > > + if (!alloc().ensureBallast()) > + return false; I think I already landed this change, otherwise this belongs in a different patch, with my blessing.

Update. Getting closer. Now works with loopdepth and inlining of calls. Some TODO's left and cleanup.

- analyzeNewLoopTypes works again. - TableSwitch optimization which replaces the input with constant works again - We now improve types at the false branch in MTest. (We didn't before) Todo: - Get "stackPhiCount" correct again. - Enable the moveBlockToEnd optimization in IonControlFlow - Create separate TempAlloc for ControlFlowGenerator - Remove the 'new' statements

Merge has happened. Moving the bugs from P2 to P1.

I think it is ready. I only have to rebase to inbound, because this is based on FF50 and I don't think they will do a dot release for this.

Updated version: - Rebased to tip (was easier than expected) - Got AnalyzeArgumentsUsage working again - No use of SystemAllocPolicy anymore Still left: - Make sure backup.restore works. It might already be ok, since it doesn't have to revert the "cfgBlock" we are generating. But in this patch or follow-up bug it would be good to let restartLoop do the same.

This applies on tip, but there happens to be a failure left. Now I have been trying to reproduce it, but I cannot get the failure to happen when I'm watching. Therefore I'm wondering if it would be possible to run this patch temporarily by the fuzzers and hopefully get a testcase that is reproducible? I don't mind the testcase is not reduced. I just need to be able to run it under gdb/rr. In particular I'm searching for an MOZ_ASSERT happening in IonAnalysis.cpp CheckUse or AssertBasicGraphCoherency. If that is possible that would be awesome. Else I will find another way to squash that bug. Thanks!

The assert I'm specifically looking at is: Assertion failure: usesBalance <= 0 (More use checks than operand checks)

There is an existing bug with the same assertion failure, bug 1314172. It will be difficult to separate out the testcases hitting that bug with the assert for this one. Shall we wait till that bug is fixed first?

Bug 1314172 is an oomTest bug with oomTest on the stack, so it should be possible to find other asserts that look the same if they are not caused by OOM.

Thanks for the advice. Funnily bug 131472 depends on this bug currently. I'll see if I find another way to find the issue. Thanks!

One of the issues I'm hitting with the "uses" patch. Lets get that already fixed.

Comment on attachment 8812796 [details] [diff] [review] Split control flow graph generation out of IonBuilder Review of attachment 8812796 [details] [diff] [review]: ----------------------------------------------------------------- Clearing since it would be hard to discriminate the right bug and we have already such a bug on file, which will make it even harder to find the issue.

Comment on attachment 8813111 [details] [diff] [review] Part 0.1: Make sure to report the out of memory Review of attachment 8813111 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonAnalysis.cpp @@ +4145,5 @@ > if (!builder.build()) { > + if (cx->isThrowingOverRecursed() || cx->isThrowingOutOfMemory()) > + return false; > + if (builder.abortReason() == AbortReason_Alloc) { > + ReportOutOfMemory(cx); Hum … I thought JIT Compiler OOM should be ignored instead of being reported, as the OOM is happening in code which is not in the control of the user, thus cannot be predicted by the user as well, nor caught properly.

(In reply to Nicolas B. Pierron [:nbp] from comment #16) > Comment on attachment 8813111 [details] [diff] [review] > Part 0.1: Make sure to report the out of memory > > Review of attachment 8813111 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/IonAnalysis.cpp > @@ +4145,5 @@ > > if (!builder.build()) { > > + if (cx->isThrowingOverRecursed() || cx->isThrowingOutOfMemory()) > > + return false; > > + if (builder.abortReason() == AbortReason_Alloc) { > > + ReportOutOfMemory(cx); > > Hum … I thought JIT Compiler OOM should be ignored instead of being > reported, as the OOM is happening in code which is not in the control of the > user, thus cannot be predicted by the user as well, nor caught properly. Currently it definitely isn't. We report OOM in full compilation and in one of the passes ... Consistency would currently be to report the OOM. Feel free to open a follow-up bug to discuss this issue and patch appropriately. I'm not trying to make a stand here.

Made sure it compiles on 64bit

This is an automated crash issue comment: Summary: Assertion failure: fallibleScope_ ([OOM] Cannot allocate a new chunk in an infallible scope.), at js/src/ds/LifoAlloc.cpp:105 Build version: mozilla-central revision 0534254e9a40+ Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize Runtime options: --fuzzing-safe --ion-offthread-compile=off --ion-eager min.js Testcase: var str = (3.14).toLocaleString(); Backtrace: received signal SIGSEGV, Segmentation fault. js::LifoAlloc::getOrCreateChunk (this=this@entry=0x7ffff0593640, n=n@entry=168) at js/src/ds/LifoAlloc.cpp:105 #0 js::LifoAlloc::getOrCreateChunk (this=this@entry=0x7ffff0593640, n=n@entry=168) at js/src/ds/LifoAlloc.cpp:105 #1 0x000000000060ce6b in js::LifoAlloc::allocImpl (this=0x7ffff0593640, n=168) at js/src/ds/LifoAlloc.h:225 #2 0x000000000060cf8a in js::LifoAlloc::allocInfallible (this=<optimized out>, n=<optimized out>) at js/src/ds/LifoAlloc.h:291 #3 0x00000000006478df in js::jit::TempAllocator::allocateInfallible (bytes=168, this=<optimized out>) at js/src/jit/JitAllocPolicy.h:44 #4 js::jit::TempObject::operator new (alloc=..., nbytes=168) at js/src/jit/JitAllocPolicy.h:162 #5 js::jit::MInstruction::operator new (alloc=..., nbytes=168) at js/src/jit/MIR.h:1122 #6 js::jit::MConcat::New<js::jit::MDefinition*&, js::jit::MDefinition*&> (alloc=...) at js/src/jit/MIR.h:7229 #7 js::jit::IonBuilder::binaryArithTryConcat (this=this@entry=0x7ffff00351c8, emitted=emitted@entry=0x7fffffff5f77, op=op@entry=JSOP_ADD, left=left@entry=0x7ffff003ce18, right=right@entry=0x7ffff003ced8) at js/src/jit/IonBuilder.cpp:3239 #8 0x000000000064a763 in js::jit::IonBuilder::jsop_binary_arith (this=this@entry=0x7ffff00351c8, op=op@entry=JSOP_ADD, left=0x7ffff003ce18, right=0x7ffff003ced8) at js/src/jit/IonBuilder.cpp:3445 #9 0x000000000064a9e9 in js::jit::IonBuilder::jsop_binary_arith (this=this@entry=0x7ffff00351c8, op=op@entry=JSOP_ADD) at js/src/jit/IonBuilder.cpp:3481 #10 0x00000000006ab771 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7ffff00351c8, op=op@entry=JSOP_ADD) at js/src/jit/IonBuilder.cpp:1816 #11 0x00000000006ac74a in js::jit::IonBuilder::visitBlock (this=0x7ffff00351c8, cfgblock=<optimized out>, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1541 #12 0x00000000006a2de7 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff00351c8) at js/src/jit/IonBuilder.cpp:1470 #13 0x00000000006a3c3f in js::jit::IonBuilder::build (this=0x7ffff00351c8) at js/src/jit/IonBuilder.cpp:846 #14 0x00000000006b7709 in js::jit::IonCompile (cx=cx@entry=0x7ffff6944000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=<optimized out>, recompile=<optimized out>, optimizationLevel=optimizationLevel@entry=js::jit::OptimizationLevel::Normal) at js/src/jit/Ion.cpp:2236 #15 0x00000000006b8202 in js::jit::Compile (cx=0x7ffff6944000, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2486 #16 0x00000000006b83f2 in js::jit::CanEnter (cx=cx@entry=0x7ffff6944000, state=...) at js/src/jit/Ion.cpp:2583 #17 0x0000000000b628b3 in js::RunScript (cx=cx@entry=0x7ffff6944000, state=...) at js/src/vm/Interpreter.cpp:381 #18 0x0000000000b62b95 in js::InternalCallOrConstruct (cx=0x7ffff6944000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:477 #19 0x0000000000b62e36 in InternalCall (cx=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:504 #20 0x0000000000b62f7a in js::CallFromStack (cx=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:510 #21 0x0000000000ef0cca in js::jit::DoCallFallback (cx=0x7ffff6944000, frame=0x7fffffff6ab8, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffff6a58, res=...) at js/src/jit/BaselineIC.cpp:6012 #22 0x00007ffff7e3d08a in ?? () [...] #32 0x0000000000000000 in ?? () rax 0x20288c0 33720512 rbx 0x11f7db8 18841016 rcx 0x7ffff6c28a2d 140737333332525 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7fffffff5e80 140737488313984 rsp 0x7fffffff5dc0 140737488313792 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff7fe4740 140737354024768 r10 0x58 88 r11 0x7ffff6b9f750 140737332770640 r12 0x7ffff0035000 140737220136960 r13 0x7ffff0593640 140737225766464 r14 0xa8 168 r15 0x0 0 rip 0x853860 <js::LifoAlloc::getOrCreateChunk(unsigned long)+944> => 0x853860 <js::LifoAlloc::getOrCreateChunk(unsigned long)+944>: movl $0x0,0x0 0x85386b <js::LifoAlloc::getOrCreateChunk(unsigned long)+955>: ud2

This is an automated crash issue comment: Summary: Assertion failure: fallibleScope_ ([OOM] Cannot allocate a new chunk in an infallible scope.), at js/src/ds/LifoAlloc.cpp:105 Build version: mozilla-central revision 0534254e9a40+ Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize Runtime options: --fuzzing-safe --ion-offthread-compile=off --ion-eager min.js Testcase: eval(` var assertDeepEq = (function(){ return function assertDeepEq(a, b, options) { }; })(); assertDeepEq(Reflect.construct(String, ["hello"]), new String("hello")); var d = Reflect.construct(Date, [1776, 6, 4]); `); Backtrace: received signal SIGSEGV, Segmentation fault. js::LifoAlloc::getOrCreateChunk (this=this@entry=0x7ffff0325c00, n=n@entry=48) at js/src/ds/LifoAlloc.cpp:105 #0 js::LifoAlloc::getOrCreateChunk (this=this@entry=0x7ffff0325c00, n=n@entry=48) at js/src/ds/LifoAlloc.cpp:105 #1 0x000000000060ce6b in js::LifoAlloc::allocImpl (this=0x7ffff0325c00, n=48) at js/src/ds/LifoAlloc.h:225 #2 0x0000000000c5c63f in js::LifoAlloc::alloc (n=48, this=0x7ffff0325c00) at js/src/ds/LifoAlloc.h:285 #3 js::LifoAlloc::new_<(anonymous namespace)::CompilerConstraintInstance<(anonymous namespace)::ConstraintDataFreezeObjectFlags>, js::LifoAlloc*&, js::HeapTypeSetKey&, (anonymous namespace)::ConstraintDataFreezeObjectFlags> (this=0x7ffff0325c00) at js/src/ds/LifoAlloc.h:454 #4 js::TypeSet::ObjectKey::hasFlags (this=<optimized out>, constraints=0x7ffff69af1e0, flags=131072) at js/src/vm/TypeInference.cpp:1826 #5 0x0000000000c5c7ad in js::TemporaryTypeSet::hasObjectFlags (this=0x7ffff69b2980, constraints=0x7ffff69af1e0, flags=flags@entry=131072) at js/src/vm/TypeInference.cpp:1852 #6 0x000000000075020d in js::jit::ElementAccessIsPacked (constraints=<optimized out>, obj=obj@entry=0x7ffff69e9818) at js/src/jit/MIR.cpp:6006 #7 0x0000000000677b06 in js::jit::IonBuilder::jsop_getelem_dense (this=0x7ffff69af278, obj=0x7ffff69e9818, index=0x7ffff0529d70, unboxedType=<optimized out>) at js/src/jit/IonBuilder.cpp:8223 #8 0x0000000000678272 in js::jit::IonBuilder::getElemTryDense (this=this@entry=0x7ffff69af278, emitted=emitted@entry=0x7fffffffb377, obj=obj@entry=0x7ffff69e9818, index=index@entry=0x7ffff0529d70) at js/src/jit/IonBuilder.cpp:7857 #9 0x0000000000698c63 in js::jit::IonBuilder::jsop_getelem (this=this@entry=0x7ffff69af278) at js/src/jit/IonBuilder.cpp:7402 #10 0x00000000006ab310 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7ffff69af278, op=op@entry=JSOP_GETELEM) at js/src/jit/IonBuilder.cpp:2139 #11 0x00000000006ac74a in js::jit::IonBuilder::visitBlock (this=0x7ffff69af278, cfgblock=<optimized out>, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1541 #12 0x00000000006a2de7 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff69af278) at js/src/jit/IonBuilder.cpp:1470 #13 0x00000000006a3c3f in js::jit::IonBuilder::build (this=0x7ffff69af278) at js/src/jit/IonBuilder.cpp:846 #14 0x00000000006b7709 in js::jit::IonCompile (cx=cx@entry=0x7ffff6944000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x7fffffffb9c8, osrPc=<optimized out>, recompile=<optimized out>, optimizationLevel=optimizationLevel@entry=js::jit::OptimizationLevel::Normal) at js/src/jit/Ion.cpp:2236 #15 0x00000000006b8202 in js::jit::Compile (cx=cx@entry=0x7ffff6944000, script=script@entry=..., osrFrame=osrFrame@entry=0x7fffffffb9c8, osrPc=osrPc@entry=0x0, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2486 #16 0x00000000006b8b12 in BaselineCanEnterAtEntry (frame=0x7fffffffb9c8, script=..., cx=0x7ffff6944000) at js/src/jit/Ion.cpp:2615 #17 js::jit::IonCompileScriptForBaseline (cx=0x7ffff6944000, frame=0x7fffffffb9c8, pc=<optimized out>) at js/src/jit/Ion.cpp:2738 #18 0x00007ffff7e40410 in ?? () [...] #51 0x00007fffffffbdb0 in ?? () #52 0x0000000000ec4115 in EnterBaseline (cx=0x7ffff6a00118, data=...) at js/src/jit/BaselineJIT.cpp:157 Backtrace stopped: frame did not save the PC rax 0x20288c0 33720512 rbx 0x11f7db8 18841016 rcx 0x7ffff6c28a2d 140737333332525 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7fffffffb1c0 140737488335296 rsp 0x7fffffffb100 140737488335104 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff7fe4740 140737354024768 r10 0x58 88 r11 0x7ffff6b9f750 140737332770640 r12 0x7ffff0522000 140737225302016 r13 0x7ffff0325c00 140737223220224 r14 0x30 48 r15 0x0 0 rip 0x853860 <js::LifoAlloc::getOrCreateChunk(unsigned long)+944> => 0x853860 <js::LifoAlloc::getOrCreateChunk(unsigned long)+944>: movl $0x0,0x0 0x85386b <js::LifoAlloc::getOrCreateChunk(unsigned long)+955>: ud2

This is an automated crash issue comment: Summary: Assertion failure: fallibleScope_ ([OOM] Cannot allocate a new chunk in an infallible scope.), at js/src/ds/LifoAlloc.cpp:105 Build version: mozilla-central revision 0534254e9a40+ Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize Runtime options: --fuzzing-safe --ion-eager min.js Testcase: var f2 = function() { return gczeal(2); }; if (f2() !== true) {} Intl.Collator.prototype.compare("a", "b"); Backtrace: received signal SIGSEGV, Segmentation fault. js::LifoAlloc::getOrCreateChunk (this=this@entry=0x7ffff69421c0, n=n@entry=24) at js/src/ds/LifoAlloc.cpp:105 #0 js::LifoAlloc::getOrCreateChunk (this=this@entry=0x7ffff69421c0, n=n@entry=24) at js/src/ds/LifoAlloc.cpp:105 #1 0x000000000060ce6b in js::LifoAlloc::allocImpl (this=0x7ffff69421c0, n=24) at js/src/ds/LifoAlloc.h:225 #2 0x000000000060cf8a in js::LifoAlloc::allocInfallible (this=<optimized out>, n=<optimized out>) at js/src/ds/LifoAlloc.h:291 #3 0x00000000006bf031 in js::jit::TempAllocator::allocateInfallible (bytes=24, this=<optimized out>) at js/src/jit/JitAllocPolicy.h:44 #4 js::jit::TempObject::operator new (alloc=..., nbytes=24) at js/src/jit/JitAllocPolicy.h:162 #5 js::jit::IonBuilder::bytecodeSite (this=0x7ffff044d1c8, pc=0x7ffff6922adc "V") at js/src/jit/IonBuilder.h:998 #6 0x00000000006ac82b in js::jit::IonBuilder::visitBlock (this=0x7ffff044d1c8, cfgblock=<optimized out>, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1590 #7 0x00000000006a2de7 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff044d1c8) at js/src/jit/IonBuilder.cpp:1470 #8 0x00000000006a3c3f in js::jit::IonBuilder::build (this=0x7ffff044d1c8) at js/src/jit/IonBuilder.cpp:846 #9 0x00000000006b7709 in js::jit::IonCompile (cx=cx@entry=0x7ffff6944000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=<optimized out>, recompile=<optimized out>, optimizationLevel=optimizationLevel@entry=js::jit::OptimizationLevel::Normal) at js/src/jit/Ion.cpp:2236 #10 0x00000000006b8202 in js::jit::Compile (cx=0x7ffff6944000, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2486 #11 0x00000000006b83f2 in js::jit::CanEnter (cx=cx@entry=0x7ffff6944000, state=...) at js/src/jit/Ion.cpp:2583 #12 0x0000000000b628b3 in js::RunScript (cx=cx@entry=0x7ffff6944000, state=...) at js/src/vm/Interpreter.cpp:381 #13 0x0000000000b62b95 in js::InternalCallOrConstruct (cx=0x7ffff6944000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:477 #14 0x0000000000b62e36 in InternalCall (cx=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:504 #15 0x0000000000b62f7a in js::CallFromStack (cx=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:510 #16 0x0000000000ef0cca in js::jit::DoCallFallback (cx=0x7ffff6944000, frame=0x7fffffff6498, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffff6438, res=...) at js/src/jit/BaselineIC.cpp:6012 #17 0x00007ffff7e3d08a in ?? () [...] #27 0x0000000000000000 in ?? () rax 0x20288c0 33720512 rbx 0x11f7db8 18841016 rcx 0x7ffff6c28a2d 140737333332525 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7fffffff5920 140737488312608 rsp 0x7fffffff5860 140737488312416 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff7fe4740 140737354024768 r10 0x58 88 r11 0x7ffff6b9f750 140737332770640 r12 0x7ffff044d000 140737224429568 r13 0x7ffff69421c0 140737330291136 r14 0x18 24 r15 0x0 0 rip 0x853860 <js::LifoAlloc::getOrCreateChunk(unsigned long)+944> => 0x853860 <js::LifoAlloc::getOrCreateChunk(unsigned long)+944>: movl $0x0,0x0 0x85386b <js::LifoAlloc::getOrCreateChunk(unsigned long)+955>: ud2

This is an automated crash issue comment: Summary: Assertion failure: exprStack == stackDepth, at js/src/jit/Recover.cpp:110 Build version: mozilla-central revision 0534254e9a40+ Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize Runtime options: --fuzzing-safe --ion-offthread-compile=off min.js Testcase: assertEq((function() { var $1 = 0, label = 0; while (1) { switch (label | 0) { case 1: case 1: return reportCompare(expect, actual, status); } } })()(), 1); Backtrace: received signal SIGSEGV, Segmentation fault. js::jit::MResumePoint::writeRecoverData (this=0x7ffff0332680, writer=...) at js/src/jit/Recover.cpp:110 #0 js::jit::MResumePoint::writeRecoverData (this=0x7ffff0332680, writer=...) at js/src/jit/Recover.cpp:110 #1 0x00000000007d5028 in js::jit::RecoverWriter::writeInstruction (this=this@entry=0x7ffff0337c48, rp=<optimized out>) at js/src/jit/Snapshots.cpp:722 #2 0x000000000080efaf in js::jit::CodeGeneratorShared::encode (this=0x7ffff0337000, recover=recover@entry=0x7ffff69ba110) at js/src/jit/shared/CodeGenerator-shared.cpp:570 #3 0x0000000000819c13 in js::jit::CodeGeneratorShared::encode (recover=<optimized out>, this=0x7ffff0337000) at js/src/jit/shared/CodeGenerator-shared.cpp:595 #4 js::jit::CodeGeneratorShared::encode (this=0x7ffff0337000, snapshot=0x7ffff69ba158) at js/src/jit/shared/CodeGenerator-shared.cpp:584 #5 0x0000000000819d36 in js::jit::CodeGeneratorShared::encode (this=this@entry=0x7ffff0337000, snapshot=snapshot@entry=0x7ffff69ba158) at js/src/jit/shared/CodeGenerator-shared.cpp:625 #6 0x00000000008d63b1 in js::jit::CodeGeneratorX86Shared::bailout<js::jit::BailoutLabel> (this=0x7ffff0337000, binder=..., snapshot=0x7ffff69ba158) at js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp:532 #7 0x00000000008a617d in js::jit::CodeGeneratorX86Shared::bailoutFrom (this=this@entry=0x7ffff0337000, label=label@entry=0x7fffffffcad0, snapshot=<optimized out>) at js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp:580 #8 0x00000000005ff392 in js::jit::CodeGenerator::emitDebugForceBailing (this=0x7ffff0337000, lir=0x7ffff69ba0b0) at js/src/jit/CodeGenerator.cpp:5055 #9 0x0000000000604ce1 in js::jit::CodeGenerator::generateBody (this=this@entry=0x7ffff0337000) at js/src/jit/CodeGenerator.cpp:5152 #10 0x000000000060592e in js::jit::CodeGenerator::generate (this=this@entry=0x7ffff0337000) at js/src/jit/CodeGenerator.cpp:9386 #11 0x000000000064445a in js::jit::GenerateCode (mir=mir@entry=0x7ffff032e278, lir=0x7ffff69b8620) at js/src/jit/Ion.cpp:2008 #12 0x00000000006b6fd6 in js::jit::CompileBackEnd (mir=mir@entry=0x7ffff032e278) at js/src/jit/Ion.cpp:2030 #13 0x00000000006b7adb in js::jit::IonCompile (cx=cx@entry=0x7ffff6944000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x7fffffffd1a8, osrPc=<optimized out>, recompile=<optimized out>, optimizationLevel=optimizationLevel@entry=js::jit::OptimizationLevel::Normal) at js/src/jit/Ion.cpp:2304 #14 0x00000000006b8202 in js::jit::Compile (cx=cx@entry=0x7ffff6944000, script=script@entry=..., osrFrame=osrFrame@entry=0x7fffffffd1a8, osrPc=osrPc@entry=0x7ffff6937d0d "\343\201?\b\377\377\377\303晑\210\b\226\210\b/B\235\005\210\b\231\004\220n2\002r\aw\006̐\220\220\210\020و\a\233\v\232\004\210\vɘ\v", forceRecompile=<optimized out>) at js/src/jit/Ion.cpp:2486 #15 0x00000000006b8c80 in BaselineCanEnterAtBranch (pc=0x7ffff6937d0d "\343\201?\b\377\377\377\303晑\210\b\226\210\b/B\235\005\210\b\231\004\220n2\002r\aw\006̐\220\220\210\020و\a\233\v\232\004\210\vɘ\v", osrFrame=0x7fffffffd1a8, script=..., cx=<optimized out>) at js/src/jit/Ion.cpp:2677 #16 js::jit::IonCompileScriptForBaseline (cx=0x7ffff6944000, frame=frame@entry=0x7fffffffd1a8, pc=pc@entry=0x7ffff6937d0d "\343\201?\b\377\377\377\303晑\210\b\226\210\b/B\235\005\210\b\231\004\220n2\002r\aw\006̐\220\220\210\020و\a\233\v\232\004\210\vɘ\v") at js/src/jit/Ion.cpp:2735 #17 0x0000000000ecf242 in js::jit::DoWarmUpCounterFallbackOSR (cx=0x7ffff6944000, frame=0x7fffffffd1a8, stub=0x7ffff0336318, infoPtr=0x7fffffffd170) at js/src/jit/BaselineIC.cpp:143 #18 0x00007ffff7e3db24 in ?? () [...] #29 0x0000000000000000 in ?? () rax 0x20288c0 33720512 rbx 0x7ffff0337c48 140737223294024 rcx 0x11e76a0 18773664 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7fffffffc8c0 140737488341184 rsp 0x7fffffffc870 140737488341104 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff7fe4740 140737354024768 r10 0x58 88 r11 0x7ffff6b9f750 140737332770640 r12 0x7ffff6937cee 140737330248942 r13 0x7ffff0332680 140737223272064 r14 0x7ffff06801a8 140737226736040 r15 0x7ffff06995c0 140737226839488 rip 0x7afd65 <js::jit::MResumePoint::writeRecoverData(js::jit::CompactBufferWriter&) const+709> => 0x7afd65 <js::jit::MResumePoint::writeRecoverData(js::jit::CompactBufferWriter&) const+709>: movl $0x0,0x0 0x7afd70 <js::jit::MResumePoint::writeRecoverData(js::jit::CompactBufferWriter&) const+720>: ud2

This is an automated crash issue comment: Summary: Assertion failure: fallibleScope_ ([OOM] Cannot allocate a new chunk in an infallible scope.), at js/src/ds/LifoAlloc.cpp:105 Build version: mozilla-central revision 0534254e9a40+ Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize Runtime options: --fuzzing-safe --ion-eager min.js Testcase: var locales = ["zh-Hant-TW", ]; var count = Intl.Collator.supportedLocalesOf(locales).length; Backtrace: received signal SIGSEGV, Segmentation fault. js::LifoAlloc::getOrCreateChunk (this=this@entry=0x7ffff69424c0, n=n@entry=16) at js/src/ds/LifoAlloc.cpp:105 #0 js::LifoAlloc::getOrCreateChunk (this=this@entry=0x7ffff69424c0, n=n@entry=16) at js/src/ds/LifoAlloc.cpp:105 #1 0x00000000007884dd in js::LifoAlloc::allocImpl (n=16, this=0x7ffff69424c0) at js/src/ds/LifoAlloc.h:225 #2 js::LifoAlloc::alloc (n=16, this=0x7ffff69424c0) at js/src/ds/LifoAlloc.h:285 #3 js::LifoAlloc::new_<js::TemporaryTypeSet, js::LifoAlloc*&, js::TypeSet::Type>(js::LifoAlloc*&, js::TypeSet::Type&&) (this=0x7ffff69424c0) at js/src/ds/LifoAlloc.h:454 #4 0x000000000075f5cc in MakeSingletonTypeSetFromKey (constraints=<optimized out>, key=0x7ffff06b6381) at js/src/jit/MIR.cpp:871 #5 0x000000000075f96a in js::jit::MakeSingletonTypeSet (obj=<optimized out>, constraints=0x7fffefba4130) at js/src/jit/MIR.cpp:877 #6 js::jit::MConstant::MConstant (this=0x7fffefa39f58, vp=..., constraints=0x7fffefba4130) at js/src/jit/MIR.cpp:944 #7 0x000000000075fce1 in js::jit::MConstant::New (alloc=..., v=..., constraints=0x7fffefba4130) at js/src/jit/MIR.cpp:806 #8 0x0000000000650d60 in js::jit::IonBuilder::constant (this=0x7fffffff6fb0, v=...) at js/src/jit/IonBuilder.cpp:13017 #9 0x00000000006a66f1 in js::jit::IonBuilder::inlineCallsite (this=0x7fffffff6fb0, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:4224 #10 0x00000000006a6c35 in js::jit::IonBuilder::jsop_call (this=this@entry=0x7fffffff6fb0, argc=2, constructing=<optimized out>) at js/src/jit/IonBuilder.cpp:5198 #11 0x00000000006ab2f0 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffffff6fb0, op=op@entry=JSOP_CALL) at js/src/jit/IonBuilder.cpp:2037 #12 0x00000000006ac74a in js::jit::IonBuilder::visitBlock (this=0x7fffffff6fb0, cfgblock=<optimized out>, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1541 #13 0x00000000006a2de7 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffffff6fb0) at js/src/jit/IonBuilder.cpp:1470 #14 0x00000000006a443d in js::jit::IonBuilder::buildInline (this=0x7fffffff6fb0, callerBuilder=<optimized out>, callerResumePoint=<optimized out>, callInfo=...) at js/src/jit/IonBuilder.cpp:1018 #15 0x00000000006a498f in js::jit::IonBuilder::inlineScriptedCall (this=0x7fffffff7a20, callInfo=..., target=<optimized out>) at js/src/jit/IonBuilder.cpp:3656 #16 0x00000000006a672b in js::jit::IonBuilder::inlineCallsite (this=0x7fffffff7a20, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:4230 #17 0x00000000006a6c35 in js::jit::IonBuilder::jsop_call (this=this@entry=0x7fffffff7a20, argc=0, constructing=<optimized out>) at js/src/jit/IonBuilder.cpp:5198 #18 0x00000000006ab2f0 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffffff7a20, op=op@entry=JSOP_CALL) at js/src/jit/IonBuilder.cpp:2037 #19 0x00000000006ac74a in js::jit::IonBuilder::visitBlock (this=0x7fffffff7a20, cfgblock=<optimized out>, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1541 #20 0x00000000006a2de7 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffffff7a20) at js/src/jit/IonBuilder.cpp:1470 #21 0x00000000006a443d in js::jit::IonBuilder::buildInline (this=0x7fffffff7a20, callerBuilder=<optimized out>, callerResumePoint=<optimized out>, callInfo=...) at js/src/jit/IonBuilder.cpp:1018 #22 0x00000000006a498f in js::jit::IonBuilder::inlineScriptedCall (this=0x7fffffff8490, callInfo=..., target=<optimized out>) at js/src/jit/IonBuilder.cpp:3656 #23 0x00000000006a672b in js::jit::IonBuilder::inlineCallsite (this=0x7fffffff8490, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:4230 #24 0x00000000006a6c35 in js::jit::IonBuilder::jsop_call (this=this@entry=0x7fffffff8490, argc=1, constructing=<optimized out>) at js/src/jit/IonBuilder.cpp:5198 #25 0x00000000006ab2f0 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffffff8490, op=op@entry=JSOP_CALL) at js/src/jit/IonBuilder.cpp:2037 #26 0x00000000006ac74a in js::jit::IonBuilder::visitBlock (this=0x7fffffff8490, cfgblock=<optimized out>, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1541 #27 0x00000000006a2de7 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffffff8490) at js/src/jit/IonBuilder.cpp:1470 #28 0x00000000006a443d in js::jit::IonBuilder::buildInline (this=0x7fffffff8490, callerBuilder=<optimized out>, callerResumePoint=<optimized out>, callInfo=...) at js/src/jit/IonBuilder.cpp:1018 #29 0x00000000006a498f in js::jit::IonBuilder::inlineScriptedCall (this=0x7fffefba41c8, callInfo=..., target=<optimized out>) at js/src/jit/IonBuilder.cpp:3656 #30 0x00000000006a672b in js::jit::IonBuilder::inlineCallsite (this=0x7fffefba41c8, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:4230 #31 0x00000000006a6c35 in js::jit::IonBuilder::jsop_call (this=this@entry=0x7fffefba41c8, argc=1, constructing=<optimized out>) at js/src/jit/IonBuilder.cpp:5198 #32 0x00000000006ab2f0 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffefba41c8, op=op@entry=JSOP_CALL) at js/src/jit/IonBuilder.cpp:2037 #33 0x00000000006ac74a in js::jit::IonBuilder::visitBlock (this=0x7fffefba41c8, cfgblock=<optimized out>, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1541 #34 0x00000000006a2de7 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffefba41c8) at js/src/jit/IonBuilder.cpp:1470 #35 0x00000000006a3c3f in js::jit::IonBuilder::build (this=0x7fffefba41c8) at js/src/jit/IonBuilder.cpp:846 #36 0x00000000006b7709 in js::jit::IonCompile (cx=cx@entry=0x7ffff6944000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=<optimized out>, recompile=<optimized out>, optimizationLevel=optimizationLevel@entry=js::jit::OptimizationLevel::Normal) at js/src/jit/Ion.cpp:2236 #37 0x00000000006b8202 in js::jit::Compile (cx=0x7ffff6944000, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2486 #38 0x00000000006b83f2 in js::jit::CanEnter (cx=cx@entry=0x7ffff6944000, state=...) at js/src/jit/Ion.cpp:2583 #39 0x0000000000b628b3 in js::RunScript (cx=cx@entry=0x7ffff6944000, state=...) at js/src/vm/Interpreter.cpp:381 #40 0x0000000000b62b95 in js::InternalCallOrConstruct (cx=0x7ffff6944000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:477 #41 0x0000000000b62e36 in InternalCall (cx=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:504 #42 0x0000000000b62f7a in js::CallFromStack (cx=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:510 #43 0x0000000000ef0cca in js::jit::DoCallFallback (cx=0x7ffff6944000, frame=0x7fffffff9668, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffff9610, res=...) at js/src/jit/BaselineIC.cpp:6012 #44 0x00007ffff7e3d08a in ?? ()

Here we go. Finally ready for review. This introduces: - CFGGenerator: Iterates the bytecode snooping for control flow. It supports all cases needed for ionmonkey. I.e. No support for catch blocks, which would be needed if we want to make this generation more global and usable by others. Most code is just a copy from IonBuilder, except for the TableSwitch and CondSwitch which I have improved. We keep the same information in the CFGStack for both now. - CFGGraph: Contains a list of CFGBlocks in RPO. - CFGBlock: Contains start/end pc and a control instruction to know what the Adjusts: - IonBuilder: It iterates over the CFGGraph. It gets the MBasicBlock corresponding to a CFGBlock and creates the instructions / successors blocks based on the "jsbytecode". The successor blocks are linked with the CFGBlocks with the CFGBlock id. - IonBuilder: Added improveTypes to the "false" branches of tests. - Saves/caches the CFGGraph on the BaselineScript if possible. (AnalyzeArguments cannot use that). - We now don't add the blocks to the graph, until iterating them. - We now add the optimization of replacing the tableswitch operand in the bodies in all cases.

This looks great. Before I review it, some high-level comments based on the summary: * We cache data on the BaselineScript? Do we ever purge this? We should probably delete it on GC, or at least shrinking GCs (GC is suppressed in IonBuilder so this should work). * We should report how much memory it uses (BaselineScript::addSizeOfIncludingThis) (especially if we don't purge it). * Out of curiosity: how much time do we spend in IonBuilder before/after on Octane? And how much time in the CFGGenerator?

Thinking about this more, we do discard most Baseline scripts regularly, so it's probably fine to keep it. Let's add a memory reporter though - this can be a follow-up patch.

Comment on attachment 8813616 [details] [diff] [review] Split control flow graph generation out of IonBuilder Review of attachment 8813616 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/JitSpewer.cpp @@ +297,4 @@ > jsonSpewer_.spewMIR(graph_); > jsonSpewer_.spewLIR(graph_); > jsonSpewer_.endPass(); > + */ Undo

During testing I always saw this was an improvement: Without: > $ TLLOG=IonCompilation TLOPTIONS=EnableGraph,EnableMainThread js run.js > $ python ~/Build/tracelogger/tools/execute.py overview $JS/dist/bin/js /tmp/tl-data.json > { > "engineOverview": { > "IonCompilation": 2261272149, > "TraceLogger overhead": 140418 > }, > "engineAmount": { > "IonCompilation": 3044, > "TraceLogger overhead": 7 > }, > "scriptOverview": {}, > "scriptTimes": {} > } With patch: > $ TLLOG=IonCompilation TLOPTIONS=EnableGraph,EnableMainThread js run.js > $ python ~/Build/tracelogger/tools/execute.py overview $JS/dist/bin/js /tmp/tl-data.json > { > "engineOverview": { > "IonCompilation": 2078843753, > "TraceLogger overhead": 92336 > }, > "engineAmount": { > "IonCompilation": 3003, > "TraceLogger overhead": 7 > }, > "scriptOverview": {}, > "scriptTimes": {} > } It shows consistently a >5% decrease in compilation time (on thread compilation time, meaning IonBuilder).

Comment on attachment 8813616 [details] [diff] [review] Split control flow graph generation out of IonBuilder Review of attachment 8813616 [details] [diff] [review]: ----------------------------------------------------------------- This is really nice, great to see so much code moving out of IonBuilder. Most comments are nits except for the CFG allocation one, I think we can save some memory there. Because of the large patch size I should take another look but I think this should cover most of it. ::: js/src/jit/BaselineJIT.h @@ +515,5 @@ > if (script->maybeIonScript() == ION_PENDING_SCRIPT) > script->setIonScript(nullptr, nullptr); > } > > + const ControlFlowGraph* cfg() { Nit: can mark this |const|. Also maybe s/cfg/controlFlowGraph/ ? And setControlFlowGraph below. ::: js/src/jit/IonBuilder.cpp @@ +551,5 @@ > + > + // Find the start and end pc of this loop. > + jsbytecode* start = cfgBlock->startPc(); > + const CFGBlock* endBlock = cfgBlock; > + while (endBlock->id() < cfg->numBlocks()) { Could we get rid of this loop by storing the end pc in the CFG? @@ +561,5 @@ > + break; > + } > + endBlock = cfg->block(endBlock->id() + 1); > + } > + MOZ_ASSERT (endBlock->stopIns()->getSuccessor(0) == cfgBlock); Nit: rm space after MOZ_ASSERT @@ +1362,5 @@ > > return true; > } > > +enum CFGState { Make this an enum class so we don't need the CFGState_ prefix. @@ +1401,5 @@ > + return CFGState_Abort; > + return CFGState_Alloc; > + } > + > + cfg_ = new ControlFlowGraph(); js_new. Also I think it'd be simpler if getGraph() allocated the graph and returned it. @@ +1423,5 @@ > + } > + > + ~AutoControlFlowGraph() { > + if (script->hasBaselineScript() && script->baselineScript()->cfg() == cfg_) > + return; Can we change this to: if (script->hasBaselineScript()) { MOZ_ASSERT(script->baselineScript()->cfg() == cfg_); return; } @@ +1425,5 @@ > + ~AutoControlFlowGraph() { > + if (script->hasBaselineScript() && script->baselineScript()->cfg() == cfg_) > + return; > + > + delete cfg_; js_delete @@ +1445,4 @@ > // > +// We keep a link between a CFGBlock and the entry MBasicBlock (in mworklist). > +// That vector only contains the MBasicBlocks that correspond with a CFGBlock. > +// We can create new MBasicBlocks that doesn't correspond to a CFGBlock. Nit: s/doesn't/don't/ @@ +1484,5 @@ > + > + if (restarted) { > + // Move back to the start of the loop. > + while (!mworklist[i] || mworklist[i]->isDead()) > + i--; MOZ_ASSERT(i > 0) before this. @@ +1629,5 @@ > +bool > +IonBuilder::visitGoto(CFGGoto* ins) > +{ > + // Test if this potentially was a fake loop > + // and create OSR entry if that is the case. Nit: wrap to 80 columns. @@ +1643,5 @@ > + > + size_t id = successor->id(); > + bool create = !mworklist[id] || mworklist[id]->isDead(); > + > + current->popn(ins->popAmount()); Nit: indentation is wrong. Tabs? @@ +1781,2 @@ > default: > + MOZ_CRASH("Unknown Control Instruction"); Nit: remove the default and move the MOZ_CRASH after the switch, so the compiler warns when adding a new enum value. @@ +1791,5 @@ > switch (op) { > case JSOP_NOP: > case JSOP_NOP_DESTRUCTURING: > case JSOP_LINENO: > + case JSOP_LOOPENTRY: // we shouldn't encounter this anymore? Remove the comment and if true add this op to the MOZ_CRASH cases below? @@ +2327,5 @@ > #endif > } > > +bool > +IonBuilder::restartLoop(const CFGBlock* hHeader) Is the h prefix from HIR? cfgHeader? @@ +2335,5 @@ > spew("New types at loop header, restarting loop body"); > > if (JitOptions.limitScriptSize) { > if (++numLoopRestarts_ >= MAX_LOOP_RESTARTS) > + return false; How does this work? Don't we treat this as an allocation failure? @@ +2873,5 @@ > + > + if (!jsop_compare(JSOP_STRICTEQ, lhs, rhs)) > + return false; > + MInstruction* cmpResult = current->pop()->toInstruction(); > + MOZ_ASSERT(!cmpResult->isEffectful()); Nit: fix indentation @@ +2992,5 @@ > + return true; > +} > + > +bool > +IonBuilder::visitThrow(CFGThrow *cfg_ins) Nit: CFGThrow* cfgIns @@ +11822,5 @@ > > // :TODO: if hasArguments() is true, and the script has a JSOP_SETARG, then > // convert all arg accesses to go through the arguments object. (see Bug 957475) > if (info().hasArguments()) > + return abort("NYI: arguments & setarg."); Nit: fix indentation ::: js/src/jit/IonBuilder.h @@ +981,5 @@ > > jsbytecode* pc; > MBasicBlock* current; > uint32_t loopDepth_; > + Vector<MBasicBlock*, 0, SystemAllocPolicy> mworklist; Nit: blockWorklist ::: js/src/jit/IonControlFlow.cpp @@ +53,5 @@ > + blocks_[i].id(), > + blocks_[i].startPc() - script->code(), > + blocks_[i].stopPc() - script->code()); > + > + jsbytecode *pc = blocks_[i].startPc(); Nit: jsbytecode* pc @@ +77,5 @@ > + } > +} > + > +bool > +ControlFlowGraph::init(CFGBlockVector& blocks) Can this be |const CFGBlockVector&|? Useful to make sure we don't accidentally change it instead of |blocks_|. @@ +87,5 @@ > + MOZ_ASSERT(blocks[i]->id() == i); > + CFGBlock block(blocks[i]->startPc()); > + > + block.setStopPc(blocks[i]->stopPc()); > + block.setId(i); Add CFGBlock::copyFrom(const CFGBlock& ..)? Then that function can copy everything including the control instruction. To avoid the copy when appending to the Vector, we could: blocks_.infallibleEmplaceBack(...); blocks_.back().copyFrom(blocks[i]); @@ +97,5 @@ > + return false; > + > + CFGControlInstruction* copy = nullptr; > + CFGControlInstruction* ins = blocks[i]->stopIns(); > + switch (ins->type()) { Should we add a |virtual clone() = 0| method to CFGControlInstruction? @@ +163,5 @@ > + copy = tableSwitch; > + break; > + } > + default: > + MOZ_CRASH("Unknown CFGControl."); Nit: remove the |default| so compilers will warn when the switch does not cover all cases. We could MOZ_ASSERT(copy) after the switch as an extra runtime check (or better, move the assert into setStopIns). @@ +200,5 @@ > +ControlFlowGenerator::traverseBytecode() > +{ > + blocks_.clear(); > + > + current = CFGBlock::New(alloc(), script->code()); As discussed on IRC, some places check for OOM. Maybe search for "if (!" in this file and remove the unnecessary ones. @@ +566,5 @@ > + // for (var i=0; i<1000; i++) {} > + // > + // To handle this, we create two blocks: one for the try block and one > + // for the code following the try-catch statement. Both blocks are > + // connected to the graph with an MGotoWithFake instruction that always This part of the comment should be updated or moved into IonBuilder? @@ +1176,5 @@ > + curCase = off ? curCase + off : GetNextPc(GetNextPc(curCase)); > + lastTarget = curTarget; > + } > + //if (JSOp(*curCase) == JSOP_DEFAULT) > + // nbBodies++; Nit: remove @@ +1918,5 @@ > + return ControlStatus_Error; > + } > + > + FixedList<CFGBlock*>& bodies = *state.switch_.bodies; > + for (size_t i = 0; i < tableswitch->numSuccessors(); i++) { Nit: no {} ::: js/src/jit/IonControlFlow.h @@ +8,5 @@ > +#define jit_IonControlFlow_h > + > +#include "mozilla/Array.h" > + > +#include "jsbytecode.h" Nit: \n after this to separate the different #include blocks @@ +9,5 @@ > + > +#include "mozilla/Array.h" > + > +#include "jsbytecode.h" > +#include "js/TypeDecls.h" This one should come after the jit/ includes or the #include checker will complain. @@ +42,5 @@ > + CFGControlInstruction* end; > + bool inWorkList; > + > + public: > + CFGBlock(jsbytecode* start) |explicit|, also a few times below for some instructions. @@ +44,5 @@ > + > + public: > + CFGBlock(jsbytecode* start) > + : id_(-1), > + start(start), Add stop(nullptr), @@ +58,5 @@ > + } > + jsbytecode* stopPc() const { > + return stop; > + } > + void setStopPc(jsbytecode* stopPc) { MOZ_ASSERT(stopPc); @@ +64,5 @@ > + } > + CFGControlInstruction* stopIns() const { > + return end; > + } > + void setStopIns(CFGControlInstruction* stopIns) { MOZ_ASSERT(stopIns); @@ +117,5 @@ > + > +class CFGControlInstruction : public TempObject > +{ > + public: > + enum Type { "enum class Type" so we don't need the Type_ prefix. @@ +503,5 @@ > +class ControlFlowGraph > +{ > + // Keeps the data alive until destruction. > + LifoAlloc lifoAlloc_; > + TempAllocator alloc_; Storing a LifoAlloc for each CFG is a bit wasteful (it overallocates), considering we cache it between compilations. What if we do the following: * Add a LifoAlloc to JitZone (like the optimized stub space). * Use that LifoAlloc for the CFG (including the ControlFlowGraph class itself) * In Zone::discardJitCode, at the end, purge this LifoAlloc (like we do for the optimized stub space). * When we decide to keep a BaselineScript because it's active on the stack, we can simply clear its CFG. For the memory reporting you can then report the size of the whole LifoAlloc, like we do for the optimized stubs: http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/js/src/vm/TypeInference.cpp#4422 @@ +510,5 @@ > + // a control instruction. > + Vector<CFGBlock, 4, JitAllocPolicy> blocks_; > + > + public: > + ControlFlowGraph() To avoid accidental copies: ControlFlowGraph(const ControlFlowGraph&) = delete; void operator=(const ControlFlowGraph&) = delete; Probably also add it to CFGBlock and ControlFlowGenerator. @@ +548,5 @@ > + TempAllocator& alloc() { > + return alloc_; > + } > + > + enum ControlStatus { enum class @@ +557,5 @@ > + ControlStatus_Jumped, // Parsing another branch at the same level. > + ControlStatus_None // No control flow. > + }; > + > + struct DeferredEdge// : public TempObject Remove "//" ? ::: js/src/jit/MIRGraph.cpp @@ +145,1 @@ > MIRGraph::removeBlocksAfter(MBasicBlock* start) Why are the changes to this function necessary? Because of the OSR block? Maybe "removeSuccessorBlocks" is a better name now. @@ +208,5 @@ > + } > + > + for (size_t i = 0; i < blocks.length(); i++) { > + blocks[i]->unmark(); > + } No {}

> ::: js/src/jit/IonBuilder.cpp > @@ +551,5 @@ > > + > > + // Find the start and end pc of this loop. > > + jsbytecode* start = cfgBlock->startPc(); > > + const CFGBlock* endBlock = cfgBlock; > > + while (endBlock->id() < cfg->numBlocks()) { > > Could we get rid of this loop by storing the end pc in the CFG? Yes. Fixed in this patch. > @@ +1423,5 @@ > > + } > > + > > + ~AutoControlFlowGraph() { > > + if (script->hasBaselineScript() && script->baselineScript()->cfg() == cfg_) > > + return; > > Can we change this to: > > if (script->hasBaselineScript()) { > MOZ_ASSERT(script->baselineScript()->cfg() == cfg_); > return; > } Yes we can! > @@ +2335,5 @@ > > spew("New types at loop header, restarting loop body"); > > > > if (JitOptions.limitScriptSize) { > > if (++numLoopRestarts_ >= MAX_LOOP_RESTARTS) > > + return false; > > How does this work? Don't we treat this as an allocation failure? My fault. It should call "abort();" > @@ +77,5 @@ > > + } > > +} > > + > > +bool > > +ControlFlowGraph::init(CFGBlockVector& blocks) > > Can this be |const CFGBlockVector&|? Useful to make sure we don't > accidentally change it instead of |blocks_|. +1 Before posting for the first review that is exactly what happened and took me some time to figure out. > @@ +87,5 @@ > > + MOZ_ASSERT(blocks[i]->id() == i); > > + CFGBlock block(blocks[i]->startPc()); > > + > > + block.setStopPc(blocks[i]->stopPc()); > > + block.setId(i); > > Add CFGBlock::copyFrom(const CFGBlock& ..)? Then that function can copy > everything including the control instruction. To avoid the copy when > appending to the Vector, we could: > ... Like discussed on IRC it is not possible to do this. When copying the control flow we cannot create new blocks as successors. We need to be able to take the correct already copied blocks. > @@ +503,5 @@ > > +class ControlFlowGraph > > +{ > > + // Keeps the data alive until destruction. > > + LifoAlloc lifoAlloc_; > > + TempAllocator alloc_; > > Storing a LifoAlloc for each CFG is a bit wasteful (it overallocates), > considering we cache it between compilations. What if we do the following: This is not fixed in this patch. I'll create a next patch for this (2.0 or 1.1 or something). But the idea you are proposing sounds good. > @@ +510,5 @@ > > + // a control instruction. > > + Vector<CFGBlock, 4, JitAllocPolicy> blocks_; > > + > > + public: > > + ControlFlowGraph() > > To avoid accidental copies: > > ControlFlowGraph(const ControlFlowGraph&) = delete; > void operator=(const ControlFlowGraph&) = delete; > > Probably also add it to CFGBlock and ControlFlowGenerator. CFGBlock was not possible, since it uses the copy when adding to the vector. > @@ +117,5 @@ > > + > > +class CFGControlInstruction : public TempObject > > +{ > > + public: > > + enum Type { > > "enum class Type" so we don't need the Type_ prefix. This is never used... We use the isXXX() everywhere. I tried to adjust this, but the macros weren't cooperative. I left it likes this... Thanks for the feedback! Good comments.

(In reply to Hannes Verschore [:h4writer] from comment #31) > > To avoid accidental copies: > > > > ControlFlowGraph(const ControlFlowGraph&) = delete; > > void operator=(const ControlFlowGraph&) = delete; > > > > Probably also add it to CFGBlock and ControlFlowGenerator. > > CFGBlock was not possible, since it uses the copy when adding to the vector. Vector only requires its element type be movable, so if you add a move constructor you can still delete these.

Comment on attachment 8815012 [details] [diff] [review] Part 1: Split control flow graph generation out of IonBuilder Review of attachment 8815012 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: js/src/jit/IonBuilder.cpp @@ +2862,5 @@ > + > +bool > +IonBuilder::visitCompare(CFGCompare* compare) > +{ > + MDefinition* lhs = current->pop(); Nit: indentation here and below is wrong. ::: js/src/jit/IonControlFlow.cpp @@ +2042,5 @@ > + CFGState state; > + state.state = TABLE_SWITCH; > + state.stopAt = exitpc; > + state.switch_.bodies = (FixedList<CFGBlock*>*)alloc.allocate( > + sizeof(FixedList<CFGBlock*>)); Nit: this may fit within 99 columns now. @@ +2057,5 @@ > + CFGState state; > + state.state = COND_SWITCH_CASE; > + state.stopAt = nullptr; > + state.switch_.bodies = (FixedList<CFGBlock*>*)alloc.allocate( > + sizeof(FixedList<CFGBlock*>)); And here. ::: js/src/jit/IonControlFlow.h @@ +12,5 @@ > +#include "jsbytecode.h" > + > +#include "jit/BytecodeAnalysis.h" > +#include "jit/JitAllocPolicy.h" > +#include "jit/FixedList.h" Nit: FixedList.h should be #included before JitAllocPolicy.h

Comment on attachment 8815016 [details] [diff] [review] Part 1.2: Use a separate allocator for all control flow graphs Review of attachment 8815016 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/gc/Zone.cpp @@ +265,5 @@ > + > + /* > + * Free all control flow graphs that are cached on BaselineScripts. > + * Assuming this happens on the mainthread and all control flow > + * graph reads happen on the mainthread. This is save. Nit: s/. This is save./, this is safe./ ::: js/src/jit/IonBuilder.cpp @@ +1361,5 @@ > }; > > +static CFGState > +GetOrCreateControlFlowGraph(TempAllocator& tempAlloc, JSScript* script, > + const ControlFlowGraph** cfg_out) Nit: cfgOut ::: js/src/jit/IonControlFlow.h @@ +54,5 @@ > +#ifdef DEBUG > + bool isEmpty() const { > + return allocator_.isEmpty(); > + } > +#endif Nit: isEmpty() isn't used as far as I can see. @@ +549,2 @@ > { > + TempAllocator& alloc_; I think we can remove this field and shrink the graph a little if we pass the TempAllocator& to init?

Pushed by hv1989@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a0625815bdd0 IonMonkey, part 0.1: Make sure to report the out of memory during IonBuilder, r=nbp https://hg.mozilla.org/integration/mozilla-inbound/rev/1e24f4d1c415 IonMonkey, part 0.2: Make sure debug_check_operand works again, r=nbp https://hg.mozilla.org/integration/mozilla-inbound/rev/3daa33144b64 IonMonkey, part 1.0: Split graph creation from IonBuilder, r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/2f96ca59484d IonMonkey, part 1.1: Use a separate allocator for all control flow graphs, r=jandem

https://hg.mozilla.org/mozilla-central/rev/a0625815bdd0 https://hg.mozilla.org/mozilla-central/rev/1e24f4d1c415 https://hg.mozilla.org/mozilla-central/rev/3daa33144b64 https://hg.mozilla.org/mozilla-central/rev/2f96ca59484d

Oops. That shouldn't have been pushed.

Pushed by hv1989@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/04de0a7449bc IonMonkey - Part 1.2 - Remove accidental pushed spewing, r=jandem