Closed Bug 1310155 Opened 3 years ago Closed 3 years ago

Split IonBuilder into cfg and the rest.

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(5 files, 9 obsolete files)

1.90 KB, patch
nbp
: review+
Details | Diff | Splinter Review
2.08 KB, patch
nbp
: review+
Details | Diff | Splinter Review
244.46 KB, patch
jandem
: review+
Details | Diff | Splinter Review
12.70 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.02 KB, patch
jandem
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → hv1989
Depends on: jsperf
Priority: -- → P3
Attached patch WIP (obsolete) — Splinter Review
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.
Attached patch WIP (obsolete) — Splinter Review
Update. Getting closer. Now works with loopdepth and inlining of calls. Some TODO's left and cleanup.
Attachment #8801068 - Attachment is obsolete: true
Attached patch WIP (obsolete) — Splinter Review
- 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
Attachment #8805681 - Attachment is obsolete: true
Priority: P3 → P2
Merge has happened. Moving the bugs from P2 to P1.
Priority: P2 → 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.
Attachment #8807665 - Attachment is obsolete: true
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.
Attachment #8811945 - Attachment is obsolete: true
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!
Attachment #8812154 - Attachment is obsolete: true
Attachment #8812796 - Flags: feedback?(gary)
Attachment #8812796 - Flags: feedback?(choller)
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?
Flags: needinfo?(hv1989)
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!
Flags: needinfo?(hv1989)
One of the issues I'm hitting with the "uses" patch. Lets get that already fixed.
Attachment #8813111 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8812796 - Flags: feedback?(gary)
Attachment #8812796 - Flags: feedback?(choller)
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
Attachment #8812796 - Attachment is obsolete: true
Attachment #8813111 - Flags: review?(nicolas.b.pierron) → review+
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 ?? ()
Attachment #8813215 - Flags: review?(nicolas.b.pierron)
Attachment #8813215 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8813156 - Attachment is obsolete: true
Attachment #8813616 - Flags: review?(jdemooij)
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
Blocks: 1319746
Blocks: 1319747
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 {}
Attachment #8813616 - Flags: review?(jdemooij)
> ::: 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.
Attachment #8813616 - Attachment is obsolete: true
Attachment #8814401 - Flags: review?(jdemooij)
(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.
Attachment #8814401 - Attachment is obsolete: true
Attachment #8814401 - Flags: review?(jdemooij)
Attachment #8815012 - Flags: review?(jdemooij)
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
Attachment #8815012 - Flags: review?(jdemooij) → review+
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?
Attachment #8815016 - Flags: review?(jdemooij) → review+
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
Depends on: 1322724
Depends on: 1322774
Attached patch Part 1.3Splinter Review
Oops. That shouldn't have been pushed.
Attachment #8817736 - Flags: review?(jdemooij)
Attachment #8817736 - Flags: review?(jdemooij) → review+
Depends on: 1323027
Pushed by hv1989@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04de0a7449bc
IonMonkey - Part 1.2 - Remove accidental pushed spewing, r=jandem
Depends on: 1323354
Depends on: 1323357
Depends on: 1323359
Depends on: 1323360
Blocks: jsperf
No longer depends on: jsperf
Depends on: 1322924
Depends on: 1323854
Depends on: 1324580
You need to log in before you can comment on or make changes to this bug.