Closed
Bug 1047529
Opened 10 years ago
Closed 9 years ago
IonMonkey: Move MResumePoint caller field to the basic blocks.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: nbp, Assigned: uli, Mentored)
References
Details
(Whiteboard: [good first bug][lang=c++])
Attachments
(1 file, 1 obsolete file)
9.26 KB,
patch
|
gcp
:
review+
|
Details | Diff | Splinter Review |
Currently all MResumePoint have a reference to the caller resume point. The caller resume point is the same for all resume points in the same basic block. By moving the "MRepointPoint *caller" [1] to the MBasicBlock [2], we should be able to save space in all resume points. Also, knowing that we have at least one resume point per basic block (except for AsmJS) we will save space during Ion compilations. Doing so imply to change the caller accessor [3] on resume point to use fetch the newly copied caller from the basic block on which the resume point is attached to. The basic block is available via the "block()" method [4]. [1] dxr.mozilla.org/mozilla-central/search?q="MResumePoint+*caller_" [2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIRGraph.h#542 [3] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h?from=js::jit::MResumePoint::caller#10186 [4] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h?from=js::jit::MNode::block#208
how do I fetch the resume point from the basic block on which the resume point is attached to?
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Debkanya Mazumder from comment #1) > how do I fetch the resume point from the basic block on which the resume > point is attached to? I am not sure to understand your question. The caller resume point field should be move to the basic block, so it would become a member field of the basic block instead of being a member of the resume point.
Should I move MResumePoint *caller() [1] to basic block too? [1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h?from=js::jit::MResumePoint::caller#10186
Reporter | ||
Comment 4•10 years ago
|
||
(In reply to Debkanya Mazumder from comment #3) > Should I move MResumePoint *caller() [1] to basic block too? No, but you should change it such that it get its content from the basic block instead.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Reporter | ||
Updated•9 years ago
|
Assignee: iankronquist → nobody
Comment 8•9 years ago
|
||
I want to make my first contribution towards open source by solving this bug. Can you help guiding me from where to start.
Reporter | ||
Comment 9•9 years ago
|
||
(In reply to kunal jain from comment #8) > I want to make my first contribution towards open source by solving this > bug. Can you help guiding me from where to start. Yes, follow the documentation for building SpiderMonkey [1,2], then read the first comment of this bug and attach a patch which fix it. If you have any issue, feel free to ask on IRC #jsapi [3]. [1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation [2] https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey [3] https://wiki.mozilla.org/IRC
Comment 10•9 years ago
|
||
It seems like MIR.h would need to include MIRGraph.h for this to work, but MirGraph.h already includes MIR.h. Thoughts?
Assignee | ||
Comment 11•9 years ago
|
||
I had a go at this and have attached a first patch. It removes the caller_ field from MResumePoint and adds a field callerResumePoint_ to MBasicBlock. The caller field is set for basic blocks only in MBasicBlock::inherit and MBasicBlock::inheritResumePoint. I hope that I haven't overlooked a case where it needs to be set. The jit-tests all pass.
Comment 12•9 years ago
|
||
Comment on attachment 8552085 [details] [diff] [review] caller.patch Review of attachment 8552085 [details] [diff] [review]: ----------------------------------------------------------------- I think you forgot to ask for a review on your patch.
Attachment #8552085 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 13•9 years ago
|
||
Comment on attachment 8552085 [details] [diff] [review] caller.patch Review of attachment 8552085 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Can you submit a new version of your patch, with the following comment fixed, a proper commit message. ::: js/src/jit/MIR.cpp @@ +2833,5 @@ > > +MResumePoint* > +MResumePoint::caller() const > +{ > + return block_ ? block_->callerResumePoint() : nullptr; We should never call rp->caller() on a resume point which is not attached to a basic block, no need to check for block_. ::: js/src/jit/MIRGraph.cpp @@ +499,1 @@ > MResumePoint::ResumeAt); nit: unwrap this line.
Attachment #8552085 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 14•9 years ago
|
||
Thanks! The patch has been changed as suggested and I hope the commit message has the right format.
Attachment #8552085 -
Attachment is obsolete: true
Comment 15•9 years ago
|
||
Comment on attachment 8567355 [details] [diff] [review] caller.patch Review of attachment 8567355 [details] [diff] [review]: ----------------------------------------------------------------- Carrying forward r+ from nbp.
Attachment #8567355 -
Flags: review+
Comment 17•9 years ago
|
||
Try looks pretty green, you can set the checkin-needed keyword if you think it's ready to go. Congrats on your first patch.
Assignee | ||
Comment 18•9 years ago
|
||
It seems I don't have permission to set keywords (I guess that needs assignment). Could you please set checkin-needed? Thanks!
Reporter | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff335be2c49
Assignee: nobody → uli
Keywords: checkin-needed
Comment 20•9 years ago
|
||
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6919150&repo=mozilla-inbound that started with this push
Flags: needinfo?(nicolas.b.pierron)
Comment 21•9 years ago
|
||
This wasn't at fault for the SM regressions. Re-landed. https://hg.mozilla.org/integration/mozilla-inbound/rev/1cdba634261c
https://hg.mozilla.org/mozilla-central/rev/1cdba634261c
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(nicolas.b.pierron)
You need to log in
before you can comment on or make changes to this bug.
Description
•