Closed
Bug 1217873
Opened 9 years ago
Closed 9 years ago
IonMonkey: MIPS64: Simplify BailoutStack for MIPS64
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: hev, Assigned: hev)
References
Details
Attachments
(3 files, 1 obsolete file)
7.59 KB,
patch
|
nbp
:
review+
arai
:
review+
|
Details | Diff | Splinter Review |
6.60 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
7.33 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
Looks the BailoutStack::frameClass not required on mips64. This bug clean up it and makes bailout implementation similar to x64.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8678694 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8678695 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8678696 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Updated•9 years ago
|
Attachment #8678694 -
Flags: review?(arai.unmht)
Assignee | ||
Updated•9 years ago
|
Attachment #8678695 -
Flags: review?(arai.unmht)
Assignee | ||
Updated•9 years ago
|
Attachment #8678696 -
Flags: review?(lhansen)
Updated•9 years ago
|
Attachment #8678694 -
Flags: review?(arai.unmht) → review+
Updated•9 years ago
|
Attachment #8678695 -
Flags: review?(arai.unmht) → review+
Comment 4•9 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•9 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•9 years ago
|
||
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•9 years ago
|
Status: NEW → ASSIGNED
Keywords: leave-open
Updated•9 years ago
|
Attachment #8679201 -
Flags: review?(lhansen) → review+
Updated•9 years ago
|
Attachment #8678694 -
Flags: review?(nicolas.b.pierron) → review+
Comment 9•9 years ago
|
||
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•9 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)
Comment 11•9 years ago
|
||
Part 3 changes CodeGeneratorMIPS64::visitOutOfLineBailout. I thought that's the reason of duplication in Part 2.
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/65cf26551505 https://hg.mozilla.org/integration/mozilla-inbound/rev/e28ef5d057ee
Updated•9 years ago
|
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•9 years ago
|
Keywords: leave-open
Assignee | ||
Updated•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment 14•9 years ago
|
||
bugherder uplift |
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
Comment 15•9 years ago
|
||
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.
Description
•