IonMonkey: MIPS64: Simplify BailoutStack for MIPS64

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hev, Assigned: hev)

Tracking

unspecified
mozilla44
Other
Linux
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Looks the BailoutStack::frameClass not required on mips64. This bug clean up it and makes bailout implementation similar to x64.
(Assignee)

Comment 1

2 years ago
Created attachment 8678694 [details] [diff] [review]
Part 1: Move BailoutStack
Attachment #8678694 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 2

2 years ago
Created attachment 8678695 [details] [diff] [review]
Part 2: Move visitOutOfLineBailout
Attachment #8678695 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 3

2 years ago
Created attachment 8678696 [details] [diff] [review]
Part 3: Simplify BailoutStack
Attachment #8678696 - Flags: review?(nicolas.b.pierron)
(Assignee)

Updated

2 years ago
Attachment #8678694 - Flags: review?(arai.unmht)
(Assignee)

Updated

2 years ago
Attachment #8678695 - Flags: review?(arai.unmht)
(Assignee)

Updated

2 years ago
Attachment #8678696 - Flags: review?(lhansen)

Updated

2 years ago
Attachment #8678694 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8678695 - Flags: review?(arai.unmht) → review+

Comment 4

2 years ago
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.
(Assignee)

Comment 5

2 years ago
(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!
(Assignee)

Comment 6

2 years ago
Created attachment 8679201 [details] [diff] [review]
Part 3: Simplify BailoutStack
Attachment #8678696 - Attachment is obsolete: true
Attachment #8678696 - Flags: review?(nicolas.b.pierron)
Attachment #8678696 - Flags: review?(lhansen)
Attachment #8679201 - Flags: review?(lhansen)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Keywords: leave-open

Comment 7

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

Updated

2 years ago
Attachment #8679201 - Flags: review?(lhansen) → review+
https://hg.mozilla.org/mozilla-central/rev/0dd4a8dd5618
Attachment #8678694 - Flags: review?(nicolas.b.pierron) → review+
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.
(Assignee)

Comment 10

2 years ago
(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)
Part 3 changes CodeGeneratorMIPS64::visitOutOfLineBailout.
I thought that's the reason of duplication in Part 2.
(Assignee)

Updated

2 years ago
Depends on: 1213743

Comment 12

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/65cf26551505
https://hg.mozilla.org/integration/mozilla-inbound/rev/e28ef5d057ee
Attachment #8678695 - Flags: review?(nicolas.b.pierron)
https://hg.mozilla.org/mozilla-central/rev/65cf26551505
https://hg.mozilla.org/mozilla-central/rev/e28ef5d057ee
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/65cf26551505
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e28ef5d057ee
status-b2g-v2.5: --- → fixed
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---
You need to log in before you can comment on or make changes to this bug.