Closed Bug 492478 Opened 16 years ago Closed 15 years ago

nanojit: remove LIR_calli and LIR_fcalli

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

Attached patch patch to remove LIR_calli and LIR_fcalli (obsolete) (deleted) — Splinter Review
LIR_calli and LIR_fcalli are not used in TM/nanojit. (And they don't even exist in tamarin-redux/nanojit.) This patch removes them and all the related stuff I could find. The patch includes a simple change to NativeSparc.h which is untested, but probably correct.
Attachment #376842 - Flags: review?(edwsmith)
Attachment #376842 - Flags: review?(edwsmith) → review-
Comment on attachment 376842 [details] [diff] [review] patch to remove LIR_calli and LIR_fcalli R- as a sync warning between tamarin and tracemonkey's nanojit branches. see this change in tamarin: http://hg.mozilla.org/tamarin-redux/rev/1278 its true we dont need these opcodes, but tamarin's nanojit does need to support indirect calls. suggestions? sounds like when we merge again we'll want a feature ifdef. in the mean time, what to do... delete dead code in TM and merge later with tamarin? or ifdef out?
I actually looked at tamarin-redux before doing this change. Since it lacked LIR_calli and LIR_fcalli, and the CALL_IMT and CALL_INDIRECT constants are never used (CALL_IMT appears in isInterface() but is never assigned to anything) I figured the change was safe. It's still unclear to me how the interaction between TM and Tamarin works... it's unclear to me what changes are worth attempting with TM because I don't know which ones could cause Tamarin problems.
regarding removing the actual opcodes, correct, it's a good change. regarding removing the support for indirect calls, the change is safe in Tracemonkey, but would not be safe in Tamarin once the tracemonkey and tamarin nanojit branches are merged back together. regarding the "IMT" calling convention, this is used by tamarin's support for fast interface-method dispatching, and removing it would also be safe in tracemonkey. In fact, it probably doesn't belong in nanojit in this form; the dispatch stubs we create are using a custom ABI. the IMT support could be redefined as a new abi instead of a special kind of CallInfo address. This work has been put off for a while, and the branches are slowly diverging, but both groups have a soft committment to re-converging when time permits. in summary - these are all safe to remove from tracemonkey, but when nanojit is merged in the future, some of it will have to show up again in nanojit to support tamarin. at that time, the functionality could be more clearly marked, maybe even ifdefed out... e.g. #ifdef NANOJIT_INDIRECT_CALLS. > it's unclear to me what changes are worth attempting with TM because > I don't know which ones could cause Tamarin problems. no worries! that's what reviews and IRC are for. I lurk in #tamarin and #jsapi in case of more questions.
Comment on attachment 376842 [details] [diff] [review] patch to remove LIR_calli and LIR_fcalli If I'm reading Edwin correctly, this patch should land in tracemonkey. Then when we merge, we'll get the new indirect-call support. I don't think the merge pain will be a big deal, and it's best not to have dead code sitting around.
Suits me!(In reply to comment #4) > (From update of attachment 376842 [details] [diff] [review]) > If I'm reading Edwin correctly, this patch should land in tracemonkey. Then > when we merge, we'll get the new indirect-call support. > > I don't think the merge pain will be a big deal, and it's best not to have dead > code sitting around. Suits me!
Whiteboard: checkin-needed
jorendorff, would you like to commit this? I almost have hg commit access, but not yet.
I tried to push the patch but its badly out of date.
Attached patch rebased patch (deleted) — Splinter Review
Attachment #376842 - Attachment is obsolete: true
Whiteboard: checkin-needed → fixed-in-tracemonkey
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: