1217873 - IonMonkey: MIPS64: Simplify BailoutStack for MIPS64

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

Looks the BailoutStack::frameClass not required on mips64. This bug clean up it and makes bailout implementation similar to x64.

Comment 1

[hev [:hev]]

Attachment: Part 1: Move BailoutStack

Comment 2

[hev [:hev]]

Attachment: Part 2: Move visitOutOfLineBailout

Comment 3

[hev [:hev]]

Attachment: Part 3: Simplify BailoutStack

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8678696 [details] [diff] [review]
Part 3: Simplify BailoutStack

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

::: js/src/jit/mips64/CodeGenerator-mips64.cpp
@@ +48,5 @@
>  
>  void
>  CodeGeneratorMIPS64::visitOutOfLineBailout(OutOfLineBailout* ool)
>  {
> +    masm.push(ImmWord(ool->snapshot()->snapshotOffset()));

No longer any need to keep the frame aligned to two words?

::: js/src/jit/mips64/Trampoline-mips64.cpp
@@ +617,3 @@
>      // Remove both the bailout frame and the topmost Ion frame's stack.
>      // Load frameSize from stack
> +    masm.loadPtr(Address(StackPointer, sizeOfBailoutInfo + sizeof(RegisterDump)), a1);

This is still loading the frameSize from the frame so I don't understand why you would not keep the offsetOfFrameSize() method and use that here, instead of hardcoding sizeof(RegisterDump).  Naming the offset seems like good hygiene.

Comment 5

[hev [:hev]]

(In reply to Lars T Hansen [:lth] from comment #4)
> Comment on attachment 8678696 [details] [diff] [review]
> Part 3: Simplify BailoutStack
> 
> Review of attachment 8678696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips64/CodeGenerator-mips64.cpp
> @@ +48,5 @@
> >  
> >  void
> >  CodeGeneratorMIPS64::visitOutOfLineBailout(OutOfLineBailout* ool)
> >  {
> > +    masm.push(ImmWord(ool->snapshot()->snapshotOffset()));
> 
> No longer any need to keep the frame aligned to two words?
I think no at this point. We just need make stack aligned before call c++ Bailout function.
// Stack is:
//     [frame]
//     snapshotOffset  // 8-byte, pushed here.
//     frameSize       // 8-byte, pushed in PushBailoutFrame
//     [bailoutFrame]  // (64 * 8) bytes, same above.
//     [bailoutInfo]   // 16-byte, in GenerateBailoutThunk
//                     // 16-byte aligned, call c++ Bailout

> 
> ::: js/src/jit/mips64/Trampoline-mips64.cpp
> @@ +617,3 @@
> >      // Remove both the bailout frame and the topmost Ion frame's stack.
> >      // Load frameSize from stack
> > +    masm.loadPtr(Address(StackPointer, sizeOfBailoutInfo + sizeof(RegisterDump)), a1);
> 
> This is still loading the frameSize from the frame so I don't understand why
> you would not keep the offsetOfFrameSize() method and use that here, instead
> of hardcoding sizeof(RegisterDump).  Naming the offset seems like good
> hygiene.
You are right. thanks!

Comment 6

[hev [:hev]]

Attachment: Part 3: Simplify BailoutStack

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dd4a8dd5618

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0dd4a8dd5618

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8678695 [details] [diff] [review]
Part 2: Move visitOutOfLineBailout

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

Is a patch missing?  Otherwise I do not see the point of duplicating the logic if we have no differences.

Comment 10

[hev [:hev]]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> Comment on attachment 8678695 [details] [diff] [review]
> Part 2: Move visitOutOfLineBailout
> 
> Review of attachment 8678695 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is a patch missing?  Otherwise I do not see the point of duplicating the
> logic if we have no differences.

Do you mean CodeGenerator-mips64? It's still not landed, waiting for review (bug 1213743)

Comment 11

[Tooru Fujisawa [:arai]]

Part 3 changes CodeGeneratorMIPS64::visitOutOfLineBailout.
I thought that's the reason of duplication in Part 2.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65cf26551505
https://hg.mozilla.org/integration/mozilla-inbound/rev/e28ef5d057ee

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/65cf26551505
https://hg.mozilla.org/mozilla-central/rev/e28ef5d057ee

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/65cf26551505
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e28ef5d057ee

Comment 15

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion