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)

defect
Not set
normal

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)

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?
(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
(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.
Assignee: iankronquist → nobody
I want to make my first contribution towards open source by solving this bug. Can you help guiding me from where to start.
(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
It seems like MIR.h would need to include MIRGraph.h for this to work, but MirGraph.h already includes MIR.h. Thoughts?
Attached patch caller.patch (obsolete) — Splinter Review
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 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)
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+
Attached patch caller.patchSplinter Review
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 on attachment 8567355 [details] [diff] [review]
caller.patch

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

Carrying forward r+ from nbp.
Attachment #8567355 - Flags: review+
Try looks pretty green, you can set the checkin-needed keyword if you think it's ready to go. Congrats on your first patch.
It seems I don't have permission to set keywords (I guess that needs assignment). Could you please set checkin-needed? Thanks!
Keywords: checkin-needed
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)
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
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Flags: needinfo?(nicolas.b.pierron)
You need to log in before you can comment on or make changes to this bug.