951668 - IonMonkey: Clean-up: Use AfterICSaveLive for enterFakeExitFrame and icRestoreLive.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

This follow what has been done in Bug 900285, the idea is to:
 - fold the buildFakeExitFrame with the enterFakeExitFrame
 - replace masm.PopRegsInMask(liveRegs)  by icRestoreLive(aic)

Also icRestoreLive should ensure that we pushed by expecting the phantom type (AfterICSaveLive) as argument, and modify it in debug builds to ensure that no fake exit frame can be called after the icRestoreLive.

Comment 1

[Nicolas B. Pierron [:nbp]]

I will make multiple patches to simplify the review as there is a clear pattern which appear for all C++ calls made within the IonCaches.  After multiple iterations, I even think that we should be able to abstract it with some kind of icCallVM which is inlining the function call.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: bug951668-group-leave-exit-frame.patch

- Move the adjustStack to be near callWithABI, as this is where it make sense.
 - Change the way we extract the result to avoid using the JSReturnReg when we can move things directly to the output Typed/Value register.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: bug951668-use-icRestoreLive.patch

- Use icRestoreLive
 - Move the stack assertion into icRestoreLive.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: bug951668-setFramePushed-implicit.patch

Prevent frame descriptor errors by always using the same MacroAssembler initialization pattern.

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 8349491 [details] [diff] [review]
bug951668-setFramePushed-implicit.patch

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

This is much needed. It will relieve many fears. Thanks for doing this.

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8349487 [details] [diff] [review]
bug951668-use-icRestoreLive.patch

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

Nice. A cute use of AfterICSaveLive. r=me

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 8349486 [details] [diff] [review]
bug951668-group-leave-exit-frame.patch

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

Good. r=me with the mistaken offset fixed.

::: js/src/jit/IonCaches.cpp
@@ +910,5 @@
>          // Test for failure.
>          masm.branchIfFalseBool(ReturnReg, masm.exceptionLabel());
>  
>          // Load the outparam vp[0] into output register(s).
> +        Address outparam(StackPointer, IonOOLPropertyOpExitFrameLayout::offsetOfResult());

Should read from IonOOLNativeExitFrameLazyout::offsetOfResult()

@@ +970,1 @@
>      JS_ASSERT(masm.framePushed() == initialStack);

nit: should this also move and be duplicated in both branches? OIt seems like I want that assertion close to the thing that makes it true. Even reviewing this, I went back to look for it in both branches.

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Eric Faust [:efaust] from comment #7)
> Comment on attachment 8349486 [details] [diff] [review]
> bug951668-group-leave-exit-frame.patch
> 
> Review of attachment 8349486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good. r=me with the mistaken offset fixed.
> 
> ::: js/src/jit/IonCaches.cpp
> @@ +910,5 @@
> >          // Test for failure.
> >          masm.branchIfFalseBool(ReturnReg, masm.exceptionLabel());
> >  
> >          // Load the outparam vp[0] into output register(s).
> > +        Address outparam(StackPointer, IonOOLPropertyOpExitFrameLayout::offsetOfResult());
> 
> Should read from IonOOLNativeExitFrameLazyout::offsetOfResult()

Good catch :)

> @@ +970,1 @@
> >      JS_ASSERT(masm.framePushed() == initialStack);
> 
> nit: should this also move and be duplicated in both branches? OIt seems
> like I want that assertion close to the thing that makes it true. Even
> reviewing this, I went back to look for it in both branches.

True, we should do the same thing when we abstract the call logic later.  In the mean time, looking at when initialStack was taken, this is related to the PopRegsInMask, which is the reason why I moved it into icRestoreLive.

Comment 9

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/208094b94589
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c09e7f144b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/49f4fd358fa1

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/208094b94589
https://hg.mozilla.org/mozilla-central/rev/1c09e7f144b4
https://hg.mozilla.org/mozilla-central/rev/49f4fd358fa1