Closed Bug 670827 Opened 13 years ago Closed 13 years ago

IonMonkey: Implement bailouts

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 10 obsolete files)

18.34 KB, patch
adrake
: review+
Details | Diff | Splinter Review
8.97 KB, patch
adrake
: review+
Details | Diff | Splinter Review
18.47 KB, patch
sstangl
: review+
Details | Diff | Splinter Review
21.80 KB, patch
adrake
: review+
Details | Diff | Splinter Review
15.62 KB, patch
sstangl
: review+
Details | Diff | Splinter Review
127.29 KB, patch
sstangl
: review+
Details | Diff | Splinter Review
Instructions (like add, unbox) have compare+branch patterns to ensure that our assumptions made during specialization will continue to hold. If these branches are taken they must trigger a bailout: we must immediately leave JIT code, restore interpreter state, and then continue running in the interpreter, because to continue running in JIT code would be invalid. 

Testing is pointless until this is implemented, because without bailouts it's very easy to crash IonMonkey code. This is the next high priority item after having generated code running.

-- Implementation details:

Like TraceMonkey, we don't want to generate JIT code to perform the JIT->Interpreter stack transformation. Bailouts should be rare (compilation is expensive, so we rely on semi-accurate specialization) and the memory increase would be bloatiferous. There are many guards, at least sans-Type Inference.

TraceMonkey solves this by maintaining a typed stack that mimics the interpreter state, and makes sure it's updated before every bailout point. When a bailout occurs, a function called LeaveTree() is called. This function takes as input the typed stack and a "snapshot" which describes the type of every slot. LeaveTree() uses this information and copies each entry from the typed stack to the interpreter's untyped stack.

One of the initial design goals of IonMonkey was *not* to maintain a typed stack, since the frequent stores on what we want to be the fast-path are ultimately unnecessary - they exist solely for rare cases. That means in IonMonkey, at a bailout point, all of the values the interpreter could want are scattered over random registers and stack slots. Luckily, however, we've already solved this much: during code generation, bailout-able instructions have an LSnapshot pointer. This structure answers the question, "Given interpreter slot N, in what register or stack slot is IonMonkey currently holding it?"

So now the trick is figuring out how to use that information, and TraceMonkey is good inspiration. We'll want something similar to (albeit, probably simpler than) LeaveTree(), that takes in typed stack slots, register files, and a snapshot, and converts them to the interpreter's untyped stack. We probably want to do some kind of very simple compression on the LIR snapshot, i.e. make sure registers, types, and slots only use 8-16 bits. The strange part will probably be that unlike TM, the registers need to homed somewhere so they can be read.

Here's what an add might look like:

   add eax, edx
   jo _overflow1

... rest of program ....

_overflow1:
   push <compressed_snapshot_pointer>
   jmp _bailout

_bailout:
   pushad
   push <all xmm registers>
   push esp
   call BailoutFromIon
   
The BailoutFromIon function would look like;
void BailoutFromIon(void **stack) {
    ion::Snapshot *snapshot = stack[OFFSET_TO_SNAPSHOT];
    for (each slot in snapshot) {
        Entry *entry = snapshot->get(slot);
        Value v = ConvertToValue(entry, stack);
        *cx->regs->sp++ = v;
    }
}

ConvertToValue would use the snapshot's information combined with the provided stack pointer to create a js::Value and then push it.
Oh, yeah, function calls.

In TM, these were "tree" calls and were by far the most complicated part of the tracer. Part of the reason was the interaction with inline calls, which got one big snapshot, whereas tree calls had separate snapshots. Those separate snapshots were linked together in some terrifying way. The other reason was that tree calls could be overlaid on the same frame, a problem we (blessedly) won't have except for arguments.

The first simplification we can make is to have separate snapshots for inline frames. The second simplification is to have a lookup table on each function that maps a return address to a snapshot taken at that callsite.

Given that we're doing esp-relative addressing and clobbering ebp, the calling convention will have to change a little to make this work. We'll have to push some magic implicit first argument containing a snapshot, otherwise we'd have no way to know anything about the parent frame. This snapshot would also tell us the real argc since knowing nargs isn't enough.
(In reply to comment #1)
> Oh, yeah, function calls.
> 
> In TM, these were "tree" calls and were by far the most complicated part of
> the tracer. Part of the reason was the interaction with inline calls, which
> got one big snapshot, whereas tree calls had separate snapshots. Those
> separate snapshots were linked together in some terrifying way. The other
> reason was that tree calls could be overlaid on the same frame, a problem we
> (blessedly) won't have except for arguments.

Top-level question: what do we want to happen when a bailout occurs in an inlined function? Materialize both frames for the interpreter? Do we try to get back on an optimized code path once we exit the inlined function in the interpreter?

> The first simplification we can make is to have separate snapshots for
> inline frames. The second simplification is to have a lookup table on each
> function that maps a return address to a snapshot taken at that callsite.

Could you expand on that a bit? I'm not familiar with the details of TM snapshots, so the TM-relative description is hard for me to understand.

> Given that we're doing esp-relative addressing and clobbering ebp, 

Question: are we always doing that? It might be nice for debugging to have a mode where ebp is used in the customary way.

> the
> calling convention will have to change a little to make this work. We'll
> have to push some magic implicit first argument containing a snapshot,
> otherwise we'd have no way to know anything about the parent frame. This
> snapshot would also tell us the real argc since knowing nargs isn't enough.
> Top-level question: what do we want to happen when a bailout occurs in an
> inlined function? Materialize both frames for the interpreter? Do we try to
> get back on an optimized code path once we exit the inlined function in the
> interpreter?

Yeah, we'll need to materialize both frames. I hadn't thought about returning to an optimized code path yet. We'll probably need to see if it pays off, if speculation failing after inlining doesn't invalidate the outer function as well. There would also need to be on-stack replacement points in the outer function.

> Could you expand on that a bit? I'm not familiar with the details of TM
> snapshots, so the TM-relative description is hard for me to understand.

I'm pretty sure my description was wrong anyway. For Ion it should be possible to take separate snapshots for each inlined frame, and inlined snapshots would have a "caller" pointer or something describing the calling frame's state. This should save on memory too, since we wouldn't have to take a new snapshot for each outer frame each time we need a snapshot in the inlined frame.

For non-inlined calls though, the stack will look something like:

 function A() {
    B();   <-- snapshot taken before the call
 }

<args>
<returnaddr EnterJIT>
<locals>
<args>
<returnaddr A>
<locals>
...
<snapshot pointer>
<bailout stack>

The bailout function has enough information to read the current frame, but not the frame above it. I'm not sure what the best way to deal with this is. If/when we use ebp-relative addressing, we could grab the caller's JSFunction from a fixed, negative offset from ebp, then use <returnaddr A> to lookup the snapshot taken at the callsite.

In esp-relative addressing, this would be harder. One solution would be to push the snapshot as part of the calling convention, so there's some tradeoff there, though Brian thinks it shouldn't matter if we're doing lots of inlining.

> > Given that we're doing esp-relative addressing and clobbering ebp, 
> 
> Question: are we always doing that? It might be nice for debugging to have a
> mode where ebp is used in the customary way.

We should definitely make it toggleable and see if it's worth having the 7th register. It complicates things and would almost certainly totally confuse breakpad.
I also neglected to mention what happens after the bailout function runs. Somehow we have to get back to the interpreter. Our values being on the C stack makes things a little harder. We can't just return the snapshot from IonMonkey, like LeaveTree.

We also probably shouldn't run the interpreter from *within* the bailout, because there is a bunch of stuff on the C stack and that's just weird.

One idea: the Bailout function can compute the offset to its return address, poke an address to a thunk there, and return the amount of stack to adjust. Something like:

  ...
  call BailoutFromIon      <-- actually returns to a weird thunk somewhere

_thunk:
  add esp, eax             <-- eax returned by bailout, stack adjustment amount
  mov eax, RESUME_IN_INTERPRETER
  ret

So calling into Ion code would get you some kind of ternary result: true, false, or "resume in interpreter".
(In reply to comment #4)
> So calling into Ion code would get you some kind of ternary result: true,
> false, or "resume in interpreter".

That sounds good.  I just reviewed a TI patch (bug 665815) that does exactly that after there was grief with the "running the interpreter from within the bailout" strategy.
(In reply to comment #3)
> I'm pretty sure my description was wrong anyway. For Ion it should be
> possible to take separate snapshots for each inlined frame, and inlined
> snapshots would have a "caller" pointer or something describing the calling
> frame's state. This should save on memory too, since we wouldn't have to
> take a new snapshot for each outer frame each time we need a snapshot in the
> inlined frame.

I like it.
 
> For non-inlined calls though, the stack will look something like:
> 
>  function A() {
>     B();   <-- snapshot taken before the call
>  }
> 
> <args>
> <returnaddr EnterJIT>
> <locals>
> <args>
> <returnaddr A>
> <locals>
> ...
> <snapshot pointer>
> <bailout stack>
> 
> The bailout function has enough information to read the current frame, but
> not the frame above it. 

To make sure I understand exactly what's needed here, is this right: You are talking about the basic problem about how to materialize multiple stack frames. To materialize a frame, you need the address of the frame itself, and the address of the snapshot for the bytecode PC that frame is at.

How do we know how much of the stack to materialize? Is it true that if the top-level script and A were running in the baseline compiler, we only need to materialize enough to finish |B| in the interpreter. But if they are all running typed code, do we really need to materialize all the way up, or not? How does Crankshaft deal with that?

> I'm not sure what the best way to deal with this is.
> If/when we use ebp-relative addressing, we could grab the caller's
> JSFunction from a fixed, negative offset from ebp, then use <returnaddr A>
> to lookup the snapshot taken at the callsite.

Seems like it should work.

> In esp-relative addressing, this would be harder. One solution would be to
> push the snapshot as part of the calling convention, so there's some
> tradeoff there, though Brian thinks it shouldn't matter if we're doing lots
> of inlining.

Isn't it also harder to find the stack frame itself with esp-relative addressing?

> > > Given that we're doing esp-relative addressing and clobbering ebp, 
> > 
> > Question: are we always doing that? It might be nice for debugging to have a
> > mode where ebp is used in the customary way.
> 
> We should definitely make it toggleable and see if it's worth having the 7th
> register. It complicates things and would almost certainly totally confuse
> breakpad.

If it's complicated, it might be better to just plan for that work later, but focus on the easier, more debuggable ebp solution for now.
(In reply to comment #4)
> I also neglected to mention what happens after the bailout function runs.
> Somehow we have to get back to the interpreter. Our values being on the C
> stack makes things a little harder. We can't just return the snapshot from
> IonMonkey, like LeaveTree.
> 
> We also probably shouldn't run the interpreter from *within* the bailout,
> because there is a bunch of stuff on the C stack and that's just weird.

Agreed.

> One idea: the Bailout function can compute the offset to its return address,
> poke an address to a thunk there, and return the amount of stack to adjust.
> Something like:
> 
>   ...
>   call BailoutFromIon      <-- actually returns to a weird thunk somewhere
> 
> _thunk:
>   add esp, eax             <-- eax returned by bailout, stack adjustment
> amount
>   mov eax, RESUME_IN_INTERPRETER
>   ret

ISTR that the return-address patching strategy was not that great for ARM. If we could easily return 2 values, that would be nice. Or maybe this is one of those things that should just be platform dependent.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attached patch wip v0 (obsolete) — Splinter Review
possible encoding for snapshots.
Attached patch wip v1 (obsolete) — Splinter Review
Adds SnapshotWriter class, for encoding snapshots.
Attachment #547309 - Attachment is obsolete: true
Attached patch wip v2, encode LSnapshots (obsolete) — Splinter Review
Attachment #547620 - Attachment is obsolete: true
Attached patch part 1: out-of-line paths (obsolete) — Splinter Review
Separated for sanity, this patch adds a little helper structure to CodeGeneratorShared, for out-of-line code generation. They are similar to the stub paths in JM1 or JSC. Basically, you package up all the state you want, and then after the method body is generated it will go through each out-of-line object and ask it to generate its code.
Attachment #548717 - Flags: review?(adrake)
Attached patch wip v3, getting somewhere (obsolete) — Splinter Review
Frames can now either have a size class or no size class. x64 frames do not have size classes, because there is no benefit: bailout tables are a waste of memory on x64. So now bailout tables are optional.

A guard using a bailout table looks like this:
>   jmp _table_for_frame_size + bailout_id
>
> // shared across compartment
> _table_for_frame_size + bailout_id
>   call _handler_for_frame_size
>   call _handler_for_frame_size
>   call _handler_for_frame_size
>   ...
>
> _handler_for_frame_size:
>   push frame_size
>   ...

A guard not using a bailout table looks like this:
>   jmp _bailout_N
>
>   .... (rest of method)
> // out-of-line code
> _bailout_N
>   push snapshotOffset
>   jmp _handler
>
>   .... (rest of out-of-line code)
> _handler:
>   push frameSize
>   jmp _global_bailout_handler

So, all that appears to be working, and we're now successfully calling ion::Bailout, which does not yet do anything. Steps left to go in this patch:
 * Implement ion::Bailout()'s stack re- and de-construction.
 * Implement x64.
 * Thoroughly go over the patch, document, clean up.
 * Test.
Attachment #547952 - Attachment is obsolete: true
Attachment #548717 - Attachment is obsolete: true
Attachment #548717 - Flags: review?(adrake)
Trying to break apart the snapshots patch so it's easier to review.

This patch adds a new class (surprise), CodeGenerator which derives from a platform specific code generator. This cleans up the hierarchy a little at the price of adding to it. It also adds the out-of-line paths discussed earlier.
Attachment #548888 - Flags: review?
Attachment #548888 - Flags: review? → review?(adrake)
Attached patch wip v4 (obsolete) — Splinter Review
cleaned up, now that the other stuff has been extricated
Attachment #548722 - Attachment is obsolete: true
Attached patch wip v5, x64 support (obsolete) — Splinter Review
Attachment #548962 - Attachment is obsolete: true
We need some way to find the JSScript of a global frame, so this patch changes the layout of an IonFrame. |callee| is now implicit, it is always there, even for global code (in which case it is a tagged JSScript pointer). This also means that callees live unboxed on the argument stack.
Attachment #549009 - Flags: review?(sstangl)
attach the right patch
Attachment #549009 - Attachment is obsolete: true
Attachment #549011 - Flags: review?(sstangl)
Attachment #549009 - Flags: review?(sstangl)
I realized writing the actual bailout code that it's pretty expensive and annoying to read the frames in reverse order, then reconstruct them in forward order. It's not even clear this makes sense. Why should the topmost frame's guard affect its random callers?

So this is a slight change of plans but nothing drastic. Tenatively, the bailout will push only frames associated with the guard (including inlined frames), and hijack the frame's return address to a thunk that will go to the interpreter. This will happen even for the initial frame, which would get modified in place but still get a new interpreter activation.
Comment on attachment 548888 [details] [diff] [review]
part 1: out-of-line paths

Review of attachment 548888 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/CodeGenerator.cpp
@@ +56,5 @@
> +{
> +    for (size_t i = 0; i < graph.numBlocks(); i++) {
> +        current = graph.getBlock(i);
> +        for (LInstructionIterator iter = current->begin(); iter != current->end(); iter++) {
> +            iter->accept(this);

Should the return value from this be ignored?

@@ +80,5 @@
> +    if (!code)
> +        return false;
> +
> +    if (!gen->script->ion) {
> +        gen->script->ion= IonScript::New(GetIonContext()->cx);

Nit: ion =
Attachment #548888 - Flags: review?(adrake) → review+
Comment on attachment 548915 [details] [diff] [review]
part 2: frame size classes should be optional

Review of attachment 548915 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +58,5 @@
>  {
>      // Note that this automatically sets MacroAssembler::framePushed().
> +    if (frameClass_ != FrameSizeClass::None())
> +        masm.reserveStack(frameClass_.frameSize());
> +    else

Nit: Can we avoid if (!foo) {} else {}?

@@ +77,5 @@
> +
> +    // Pop the stack we allocated at the start of the function.
> +    if (frameClass_ != FrameSizeClass::None())
> +        masm.freeStack(frameClass_.frameSize());
> +    else

And here
Attachment #548915 - Flags: review?(adrake) → review+
Comment on attachment 548938 [details] [diff] [review]
part 3: call API for macro assembler

Review of attachment 548938 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/IonMacroAssembler.cpp
@@ +55,5 @@
> +                          : 0;
> +
> +    // Find the total number of bytes the stack will have been adjusted by,
> +    // in order to compute alignment.
> +    stackAdjust_ = alignStackForCall(stackForArgs);

Assert non-zero return. (If zero returns are permitted, there should be another mechanism for preventing nested setupABICall(), but it doesn't look like they can result.)

@@ +61,5 @@
> +    reserveStack(stackAdjust_);
> +}
> +
> +void
> +MacroAssembler::setupABICall(uint32 args, const Register &scratch)

The name of this function could be changed to make its distinction from the other setupABICall() clearer.
setupABICallDynamicAlign()?

@@ +72,5 @@
> +
> +    // Find the total number of bytes the stack will have been adjusted by,
> +    // in order to compute alignment. Include a stack slot for saving the stack
> +    // pointer, which will be dynamically aligned.
> +    stackAdjust_ = alignStackForCall(stackForArgs, scratch);

Same non-zero assertion here.

@@ +79,5 @@
> +}
> +
> +void
> +MacroAssembler::setABIArg(uint32 arg, const Register &reg)
> +{

JS_ASSERT(stackAdjust_);

@@ +92,5 @@
> +}
> +
> +void
> +MacroAssembler::callWithABI(void *fun)
> +{

JS_ASSERT(stackAdjust_);

@@ +95,5 @@
> +MacroAssembler::callWithABI(void *fun)
> +{
> +    if (NumArgRegs) {
> +        // Perform argument move resolution now.
> +        enoughMemory_ &= moveResolver_.resolve();

oom() check here?

@@ +115,5 @@
> +    if (stackAdjust_)
> +        freeStack(stackAdjust_);
> +    if (dynamicAlignment_)
> +        restoreStackFromDynamicAlignment();
> +    stackAdjust_ = 0;

Resetting stackAdjust_ can be placed into the if (stackAdjust_) {} block above.
But doesn't callWithABI() require a previous call to setupABICall()? And that in turn sets non-zero stackAdjust_, which we can assert.
So I think the if (stackAdjust_) can be removed in favor of the above assertion.

::: js/src/ion/x64/Assembler-x64.h
@@ +70,5 @@
>  static const FloatRegister InvalidFloatReg = { JSC::X86Registers::invalid_xmm };
>  
>  static const Register StackPointer = rsp;
>  static const Register JSReturnReg = rcx;
> +static const Register ReturnReg = rax;

CReturnReg?

::: js/src/ion/x64/MacroAssembler-x64.h
@@ +63,5 @@
> +        uint32 displacement = total + framePushed_;
> +        return total + ComputeByteAlignment(displacement, StackAlignment);
> +    }
> +
> +    uint32 alignStackForCall(uint32 stackForArgs, const Register &scratch) {

alignDynamicStackForCall()?

@@ +75,5 @@
> +        uint32 displacement = total + STACK_SLOT_SIZE;
> +        return total + ComputeByteAlignment(displacement, StackAlignment);
> +    }
> +
> +    void restoreStackFromDynamicAlignment() {

restoreDynamicStackFromCall()?

@@ +97,5 @@
> +    }
> +    void setStackArg(const Register &reg, uint32 arg) {
> +        movq(reg, Operand(rsp, (arg - NumArgRegs) * STACK_SLOT_SIZE + ShadowStackSpace));
> +    }
> +    void checkCallAlignment() {

#ifdef DEBUG, if we want to assert alignment. Can't hurt; I'd be for it.

@@ +102,5 @@
> +        Label good;
> +        movl(rsp, rax);
> +        testq(Imm32(StackAlignment - 1), rax);
> +        j(Equal, &good);
> +        breakpoint();

Segmentation fault.

::: js/src/ion/x86/Assembler-x86.h
@@ +64,5 @@
>  
>  static const Register JSReturnReg_Type = ecx;
>  static const Register JSReturnReg_Data = edx;
>  static const Register StackPointer = esp;
> +static const Register ReturnReg = eax;

CReturnReg?

::: js/src/ion/x86/MacroAssembler-x86.h
@@ +57,5 @@
> +        uint32 displacement = stackForArgs + framePushed_;
> +        return stackForArgs + ComputeByteAlignment(displacement, StackAlignment);
> +    }
> +
> +    uint32 alignStackForCall(uint32 stackForArgs, const Register &scratch) {

Same proposed name changes as with the x64 version.

@@ +90,5 @@
> +    }
> +    void setStackArg(const Register &reg, uint32 arg) {
> +        movl(reg, Operand(esp, arg * STACK_SLOT_SIZE));
> +    }
> +    void checkCallAlignment() {

Same comments as with x64.
Attachment #548938 - Flags: review?(sstangl) → review+
Comment on attachment 549011 [details] [diff] [review]
part 5: push callee on all frames, leave unboxed

Review of attachment 549011 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/IonCompartment.h
@@ +47,5 @@
>  
>  namespace js {
>  namespace ion {
>  
> +typedef JSBool (*EnterIonCode)(void *code, int argc, Value *argv, Value *vp, void *token);

Is there a good name with more semantic meaning than 'token'? Maybe 'calleetoken'?

::: js/src/ion/IonFrames.h
@@ +49,5 @@
> +//   argN     _ 
> +//   ...       \- These are jsvals
> +//   arg0      /
> +//   this    _/
> +//   callee     - Encodes script or JSFunction

calleetoken

::: js/src/ion/MIRGraph.cpp
@@ +59,5 @@
>      fun_(fun),
>      graph_(graph),
>      error_(false)
>  {
> +    nslots_ = script->nslots + (fun ? fun->nargs + 1 : 0);

This logic occurs in a bunch of places. Instead of '1', could we use a named constant nearby a comment (maybe in IonFrames.h)?

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +56,5 @@
>  {
>      const Register reg_code = ArgReg0;
>      const Register reg_argc = ArgReg1;
>      const Register reg_argv = ArgReg2;
>      const Register reg_vp   = ArgReg3;

const Register reg_token = ArgReg4;

@@ +60,5 @@
>      const Register reg_vp   = ArgReg3;
>  
>      MacroAssembler masm(cx);
>  
> +    masm.breakpoint();

"Hm."

@@ +124,5 @@
> +    // stack space.
> +#if defined(_WIN64)
> +    masm.push(Operand(rbp, 8 + ShadowStackSpace));
> +#else
> +    masm.push(ArgReg4);

masm.push(reg_token);
Attachment #549011 - Flags: review?(sstangl) → review+
Attached patch wip v6 (obsolete) — Splinter Review
Frame restoration works though we still need to thunk back to the interpreter.
Attachment #548987 - Attachment is obsolete: true
Attachment #548939 - Flags: review?(adrake) → review+
Attached patch wip v7 (obsolete) — Splinter Review
Bailouts are ... working! Time for cleanup and x64 parity.
Attachment #550584 - Attachment is obsolete: true
Attached patch v8: workingSplinter Review
Works on both x86 and x64.
Attachment #550965 - Attachment is obsolete: true
Attachment #550993 - Flags: review?(sstangl)
Comment on attachment 550993 [details] [diff] [review]
v8: working

Review of attachment 550993 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/Bailouts.h
@@ +45,5 @@
> +#include "jstypes.h"
> +
> +#if defined(JS_CPU_X86)
> +# include "ion/x86/Bailouts-x86.h"
> +#else

#else if defined(JS_CPU_X64)
and error otherwise.

::: js/src/ion/CodeGenerator.cpp
@@ +53,5 @@
>  
>  bool
>  CodeGenerator::generateBody()
>  {
>      for (size_t i = 0; i < graph.numBlocks(); i++) {

for (MBasicBlockIterator iter(graph.begin()); iter != graph.end(); iter++) {

@@ +54,5 @@
>  bool
>  CodeGenerator::generateBody()
>  {
>      for (size_t i = 0; i < graph.numBlocks(); i++) {
>          current = graph.getBlock(i);

current = *iter;

@@ +57,5 @@
>      for (size_t i = 0; i < graph.numBlocks(); i++) {
>          current = graph.getBlock(i);
>          for (LInstructionIterator iter = current->begin(); iter != current->end(); iter++) {
> +            if (!iter->accept(this))
> +                return true;

return false;?

::: js/src/ion/Ion.cpp
@@ +153,5 @@
>  
>  void
>  IonCompartment::mark(JSTracer *trc, JSCompartment *compartment)
>  {
> +    if (compartment->active) {

if (!compartment->active)
    return;

...

@@ +462,5 @@
> +CheckFrame(StackFrame *fp)
> +{
> +    if (!fp->isFunctionFrame()) {
> +        // Support for this is almost there - we would need a new
> +        // pushBailoutFrame. For the most part we just don't support support

-support

@@ +509,5 @@
> +        return false;
> +    }
> +
> +    // This check is to not overrun the stack. Eventually, we will want to
> +    // handel this when we support JSOP_ARGUMENTS or function calls.

handle

::: js/src/ion/Snapshots.cpp
@@ +59,5 @@
> +//   [vws] A variable-width signed integer.
> +//    [u8] An 8-bit unsigned integer.
> +// [slot*] Information on how to reify a js::Value from an Ion frame. The
> +//         first byte is split as thus:
> +//           Bits 0-2: JSValueType

Static assert that a JSValueType can fit in 3 bits.

@@ +60,5 @@
> +//    [u8] An 8-bit unsigned integer.
> +// [slot*] Information on how to reify a js::Value from an Ion frame. The
> +//         first byte is split as thus:
> +//           Bits 0-2: JSValueType
> +//           Bits 3-7: 5-bit register code ("reg").

Static assert that a unique ID can be assigned to the registers given 5 bits.

@@ +220,5 @@
> +{
> +    JS_ASSERT_IF(fun, uint32(fun->nargs + 1) < SNAPSHOT_MAX_NARGS);
> +    JS_ASSERT(exprStack < SNAPSHOT_MAX_STACK);
> +
> +    uint32 formalArgs = fun ? fun->nargs + 1 : 0;

Do we have a named constant for this 1, preceded by a comment? The same calculation occurs throughout the codebase: it would be pleasant to not have duplicate logic.

::: js/src/ion/Snapshots.h
@@ +250,5 @@
> +    void addSlot(int32 valueStackSlot);
> +#endif
> +    void endSnapshot();
> +
> +    bool outOfMemory() const {

Called oom() everywhere else. We should standardize.

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +179,5 @@
> +{
> +    MacroAssembler masm(cx);
> +
> +    // Pop arguments off the stack.
> +    // eax <- 8*argc (size of all arugments we pushed on the stack)

Also some padding.

::: js/src/vm/Stack.cpp
@@ +698,5 @@
> +StackFrame *
> +ContextStack::pushBailoutFrame(JSContext *cx, JSObject *callee, JSFunction *fun,
> +                               JSScript *script, BailoutFrameGuard *bfg)
> +{
> +    uintN formalArgs = fun->nargs + 2;

The value 2, like the value 1 elsewhere in the codebase and patch, should be a constant that is either commented or constructed.
Attachment #550993 - Flags: review?(sstangl) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/8a0228f35bd4
http://hg.mozilla.org/projects/ionmonkey/rev/97cb6abb83aa
http://hg.mozilla.org/projects/ionmonkey/rev/7724af380dcc
http://hg.mozilla.org/projects/ionmonkey/rev/1493609db7d0
http://hg.mozilla.org/projects/ionmonkey/rev/a69ecadc75e1
http://hg.mozilla.org/projects/ionmonkey/rev/ad399da18b2c
http://hg.mozilla.org/projects/ionmonkey/rev/76d004ae24c5

With nits.

> for (MBasicBlockIterator iter(graph.begin()); iter != graph.end(); iter++) {
> 

This is LIR, not MIR.

> Static assert that a JSValueType can fit in 3 bits.
> Static assert that a unique ID can be assigned to the registers given 5 bits.

I put dynamic asserts in writeSlotHeader, it seems hard to static assert on the type one at least.

> > +    bool outOfMemory() const {
> 
> Called oom() everywhere else. We should standardize.

Okay, I'll take that over renaming stuff in Nitro :)

> The value 2, like the value 1 elsewhere in the codebase and patch, should be
> a constant that is either commented or constructed.

nargs + 2 is splattered all over the JS engine. For IonMonkey though it's +1, (since the callee is special) so I've introduced a local CountArgSlots for us.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.