Closed Bug 540368 Opened 10 years ago Closed 10 years ago

nanojit: split LIR_qlo, LIR_live and LIR_ret into two opcodes each to faciliate LIR type-checking

Categories

(Core Graveyard :: Nanojit, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(3 files)

LIR_qlo currently has two distinct uses.  One is in combination with LIR_qhi
and LIR_qjoin, for splitting/joining 64-bit floats.  The other is for 
truncating 64-bit integers.  Because these have different operand types
LIR_qlo cannot be type-checked (see bug 463137).  We need to split these two
uses into two different opcodes.  Furthermore, the split-float64 version
should only occur on 32-bit (softfloat) platforms, but the truncate-int64
version should only occur on 64-bit platforms.

Also, LIR_live is currently used for both 32-bit integers and 64-bit 
integers, and so also cannot be type-checked.  It should be split into 
LIR_live and LIR_qlive.  And LIR_qlive shouldn't occur on 32-bit platforms.
LIR_ret is also a problem: it is used in TM as if it takes a 32-bit integer argument, but it is used in TR as if it takes a word-sized integer argument.  We should split it into LIR_ret and LIR_qret for clarity and consistency with all the other cases, and have LIR_pret as a synonym.  (We should probably also have LIR_plive as a synonym.)
Summary: nanojit: split LIR_qlo and LIR_live into two opcodes each → nanojit: split LIR_qlo, LIR_live and LIR_ret into two opcodes each
qparam/pparam/fparam is missing too.
(In reply to comment #2)
> qparam/pparam/fparam is missing too.

We have qparam, and pparam exists as a synonym, although it's called param.  We don't have fparam, but this bug is concerned with opcodes used on multiple types rather than missing opcodes (there are quite a few of those, but they don't seem that important if they're not being used).
Summary: nanojit: split LIR_qlo, LIR_live and LIR_ret into two opcodes each → nanojit: split LIR_qlo, LIR_live and LIR_ret into two opcodes each to faciliate LIR type-checking
Blocks: 542326
Attached patch NJ patchSplinter Review
All pretty straightforward.  The ExprFilter::ins1() changes are mostly because I renamed 'i' to the clearer 'oprnd1'.
Attachment #423696 - Flags: review?(edwsmith)
Attached patch TR patchSplinter Review
Patch passes acceptance tests on i386 and X64 on my machine.  The
pret(i2p(x)) and pret(u2p(x)) cases look like they might be able to be
reduced to just ret(x) but I wasn't sure so I didn't try it.
Attachment #423697 - Flags: review?(edwsmith)
Attached patch TM patchSplinter Review
Attachment #423698 - Flags: review?(dvander)
Blocks: 538514
Comment on attachment 423696 [details] [diff] [review]
NJ patch

low-priority: on ppc64, if the input to LIR_q2i is in a GP register then it's essentially a mov.  Spilling and reloading will just insert memory traffic & stalls.  qlo had to do that (conservatively) for the case when the input is in a FP register.
Attachment #423696 - Flags: review?(edwsmith) → review+
(In reply to comment #5)
> Created an attachment (id=423697) [details]
> TR patch
> 
> Patch passes acceptance tests on i386 and X64 on my machine.  The
> pret(i2p(x)) and pret(u2p(x)) cases look like they might be able to be
> reduced to just ret(x) but I wasn't sure so I didn't try it.

On 32bit machines, they should reduce to just LIR_ret, but on 64bit machines they should cause zero-extension or sign-extension of the result.

The alternative would be to have LIR_uret and LIR_iret, which on 64bit machines would do the sign extension on ABI's that require the callee to do it.  (I haven't investigated)   ... or require LIR_ret on 64-bit cpu's to inspect the call signature and use that instead of the opcode.
Attachment #423697 - Flags: review?(edwsmith) → review+
Attachment #423698 - Flags: review?(dvander) → review+
http:///hg.mozilla.org/tracemonkey/rev/ec77f24175bf
http:///hg.mozilla.org/tracemonkey/rev/3c3b005de959
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
http://hg.mozilla.org/tamarin-redux/rev/ac8300cb5a15
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
http:///hg.mozilla.org/mozilla-central/rev/ec77f24175bf
http:///hg.mozilla.org/mozilla-central/rev/3c3b005de959
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.