Closed Bug 542326 Opened 10 years ago Closed 10 years ago

nanojit: add NJ_SOFTFLOAT_SUPPORTED, and only compile in support for non-universal opcodes on platforms that use them

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, 2 obsolete files)

AIUI ARM is the only platform that supports SoftFloat, although MIPS might if/when it's added.

Also, LIR_qlo/LIR_qhi/LIR_qjoin are only needed for SoftFloat.  (Actually, LIR_qlo has two different meanings and needs to be split, see bug 540368;  the meaning that takes a float as input is the relevant one here.)  But i386, PPC and Sparc all generate code for some or all of these opcodes.

I propose to introduce NJ_SOFTFLOAT_SUPPORTED, similar to existing #defines like NJ_F2I_SUPPORTED, NJ_JTBL_SUPPORTED.  And also to remove the code in the i386/PPC/Sparc backends for handling these opcodes.

We could also make all the SoftFloat code in TM and TR conditionally compiled on this constant, but I'm not sure if that's a good idea.

Before I do it, if anyone can see problems with this I'd like to know.
Actually, the NJ_SOFTFLOAT_SUPPORTED #define may not be necessary.  It would be enough to just put NanoAssert(0) in asm_qlo() et al for the non-SoftFloat platforms.  (Depends if we want to comment out the SoftFloat code in TM/TR or not.)
IMHO it would be nice to have all the code used for softfloat obviously segregated into a little #ifdef section; it makes it clear that the code is only used for that.
Nb: stejohns tells me that qlo/qhi/qjoin were needed for non-SoftFloat platforms at one point in tamarin-tracing, but this is no longer the case.
A strong argument in favour of having NJ_SOFTFLOAT_SUPPORTED -- it's needed for lirasm --random, so it knows whether to generate qlo/qhi/qjoin or not.  I also think I'll make asm_q{lo,hi,join}() conditionally defined, so that they don't need to be present in all backends.
Attached patch patch (obsolete) — Splinter Review
The patch adds NJ_SOFTFLOAT_SUPPORTED and is quite aggressive with it -- 
asm_q{join,lo,hi}() are only required if it's defined.  This saves the need
for some empty functions in the non-SoftFloat back-ends.

I also did something similar with NANOJIT_64BIT and asm_{promote,q2i}().

TM and TR could conditionally compile their SoftFloat code, but for the 
moment I haven't changed that.
Attachment #423934 - Flags: review?(stejohns)
Comment on attachment 423934 [details] [diff] [review]
patch

Sure would be nice if we could have LIRopcode.tbl allow for conditional compilation of opcodes, so that the opcodes in question wouldn't exist on inappopriate platforms.
Attachment #423934 - Flags: review?(stejohns) → review+
Why can't you #ifdef in the .tbl file? The C pre-processor stands ready!

/be
Nanojit requires that all opcodes 0..127 be in use, even if it's with an unused opcode.  So we'd have something like this:

 #if NJ_SOFTFLOAT_SUPPORTED
 OPDEF(qjoin,   114, Op2,  F64)
 #else
 OPDEF(__114,   114, None, Void)
 #endif

a cure that is worse than the disease.  The same story applies for lots of other opcodes, eg. all the 64-bit integer ones are never used on 32-bit platforms.
One-time def of:
#if NJ_SOFTFLOAT_SUPPORTED
#define OPDEF_IF_SOFTFLOAT OPDEF
#else
#define OPDEF_IF_SOFTFLOAT(a, b, c, d) OPDEF(__##b, b, None, Void)
#endif

and then:
OPDEF_IF_SOFTFLOAT(qjoin, 114, Op2, F64)

?

Could be the dayquil talkin', of course.
jsproto.tbl suffered the same cure/disease, so we used macro'ed harder. Did it pay off? Not sure.

/be
Hmm, Senor Dayquil might be on(to) something.

Pros:

- Compile-time guarantees that certain opcodes aren't used on platforms that don't support them.

- Smaller code size (of Nanojit itself, not the code it generates) on some platforms, esp. 32-bit ones (there are quite a few 64-bit opcodes), possibly slightly faster (opcode switches have fewer cases).  In both cases gains will likely be small, though.

- ValidateWriter wouldn't need to do run-time 32-bit/64-bit/softfloat checking.

Cons:

- More conditional compilation.  But we already have some in this manner, this would ensure that we do it consistently.


I'll give it a go and see how it feels, if it affects run-time and/or code size at all.
Señor Dayquil agrees with jsproto.tbl, for those who didn't take a look.

/be
Attached patch patch v2 (obsolete) — Splinter Review
This is a much more aggressive version that completely strips out opcodes
from platforms where they are never used.  Eg. we have:

- 32-bit only opcodes
- 64-bit only opcodes
- SoftFloat only opcodes
- i386/X64-only opcodes

It also conditionally compiles some things like ARGSIZE_Q and LTy_I64 that
only occur on 64-bit platforms.

This patch incorporates NJ and TM.  I haven't done a TR patch yet.

There's a slight reduction in the code size -- 2k for the js shell on i386,
5k on X64.

There's a slight reduction in the number of instructions executed -- 3% for
3d-raytrace.js (an extremem example), 0.3% for all of SunSpider.

It's about a 60 line reduction in Nanojit's code line count.

There are a couple of "njn" comments in LirBufWriter::insCall() where something isn't quite right, or I'm unsure.

I've asked stejohns to review, but edwsmith should also take a look at a change of this depth.
Attachment #424187 - Flags: review?(stejohns)
Comment on attachment 424187 [details] [diff] [review]
patch v2

Wow, that's a big honkin' patch. I very much like the end goal, but due to size, probably good to have another pair of eyes on it before we land it. (I'd also like to see a TR patch before landing.)

One possible error noted: in ExprFilter::ins3, I suspect NanoAssert(isCseOpcode(v)) should really be NanoAssert(isCmovOpcode(v)) ?

Also, a nit: OP___ and CL___ are awfully ugly names. I realize the motivation (allowing for OP_32 etc) and I don't have a better suggestion, but my first reaction was "surely there's a less-weird name possibility"...
Attachment #424187 - Flags: review?(stejohns) → review+
(In reply to comment #14)
> 
> One possible error noted: in ExprFilter::ins3, I suspect
> NanoAssert(isCseOpcode(v)) should really be NanoAssert(isCmovOpcode(v)) ?

Good catch!

> Also, a nit: OP___ and CL___ are awfully ugly names. I realize the motivation
> (allowing for OP_32 etc) and I don't have a better suggestion, but my first
> reaction was "surely there's a less-weird name possibility"...

I insist that whatever names are used all have the same length, so the columns in LIRopcode.tbl line up nicely.  And the "___" suffixes make the exceptional cases very obvious...
Attached patch TR patchSplinter Review
This patch passes TR acceptance tests on i386 and X64.

One thing I wasn't sure about:  I think LirHelper::stq() should really be called LirHelper::stf() because I think it only takes float64 values, not int64 values.  In which case the assertion within it should be isF64() rather than isN64().  But I wasn't certain and I figure whoever lands this patch in TR can make the decision.
Attachment #426440 - Flags: review?(stejohns)
Attached patch NJ patchSplinter Review
Since stejohns already r+'d this approach, I'm asking for a lightweight "are you ok with the general idea" review rather than a "check every single line makes sense" review.  This version is slightly different to the one stejohns reviewed just because of changes caused by other patches that have landed in the meantime.
Attachment #423934 - Attachment is obsolete: true
Attachment #424187 - Attachment is obsolete: true
Attachment #426441 - Flags: review?(edwsmith)
Summary: nanojit: add NJ_SOFTFLOAT_SUPPORTED and only handle qlo/qhi/qjoin if needed → nanojit: add NJ_SOFTFLOAT_SUPPORTED, and only compile in support for non-universal opcodes on platforms that use them
Comment on attachment 426441 [details] [diff] [review]
NJ patch

i'm okay with the general approach, i havent gone through it line by line.

In places where the extra definitions dont cost anything in code size or memory, i'd be in favor of not using the ifdefs.  just reduces eye pain.
Attachment #426441 - Flags: review?(edwsmith) → review+
Attachment #426440 - Flags: review?(stejohns) → review+
(In reply to comment #16)
> One thing I wasn't sure about:  I think LirHelper::stq() should really be
> called LirHelper::stf() because I think it only takes float64 values, not int64
> values.

Probably so. I'll look at it when this lands.
(In reply to comment #18)
> i'm okay with the general approach, i havent gone through it line by line.
> 
> In places where the extra definitions dont cost anything in code size or
> memory, i'd be in favor of not using the ifdefs.  just reduces eye pain.

That's not possible -- this is an all-or-nothing approach.  If the opcode doesn't exist on a platform then you absolutely have to #ifdef every use otherwise you'll get compile errors.

I agree it's a bit ugly at times, but currently we have a mishmash -- in some places we have assertions checking that non-supported opcodes aren't used, in others we don't.  At least things are consistent with the #ifdef approach, and it's impossible to use a non-universal opcode on an inappropriate platform.
http://hg.mozilla.org/tracemonkey/rev/0dc74fd43862
http://hg.mozilla.org/tracemonkey/rev/3d8b07cdee97
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
NativePPC.cpp needed lots of love and was missing it, both 32 and 64 bit. (This is additive to the previous patch)
Attachment #427039 - Flags: review?(edwsmith)
Attachment #427039 - Flags: review?(edwsmith) → review+
http://hg.mozilla.org/tamarin-redux/rev/ec68add0be82
http://hg.mozilla.org/tamarin-redux/rev/b8f64e82da3f
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
It appears that NativeSparc.cpp also needs some love here, but I don't have easy access to a Sparc box. Adding Leon Sha.
Looks like MIPS also needs a little help here, adding Chris Dearman

NativeMIPS.cpp:582: error: no 'void nanojit::Assembler::asm_qjoin(nanojit::LIns*)' member function declared in class 'nanojit::Assembler'
NativeMIPS.cpp:659: error: no 'void nanojit::Assembler::asm_q2i(nanojit::LIns*)' member function declared in class 'nanojit::Assembler'
NativeMIPS.cpp:664: error: no 'void nanojit::Assembler::asm_promote(nanojit::LIns*)' member function declared in class 'nanojit::Assembler'
NativeMIPS.cpp:737: error: no 'void nanojit::Assembler::asm_qhi(nanojit::LIns*)' member function declared in class 'nanojit::Assembler'
NativeMIPS.cpp:746: error: no 'void nanojit::Assembler::asm_qlo(nanojit::LIns*)' member function declared in class 'nanojit::Assembler'



NativeMIPS.cpp: In member function 'void nanojit::Assembler::asm_arith(nanojit::LIns*)':
    NativeMIPS.cpp:1079: error: 'LIR_div' was not declared in this scope
    NativeMIPS.cpp:1080: error: 'LIR_mod' was not declared in this scope
NativeMIPS.cpp: In member function 'void nanojit::Assembler::asm_store64(nanojit::LOpcode, nanojit::LIns*, int, nanojit::LIns*)':
    NativeMIPS.cpp:1118: error: 'LIR_ldq' was not declared in this scope
    NativeMIPS.cpp:1118: error: 'LIR_ldqc' wasnot declared in this scope
NativeMIPS.cpp: In member function 'nanojit::RegisterMask nanojit::Assembler::hint(nanojit::LIns*)':
    NativeMIPS.cpp:1858: error: 'LIR_callh' was not declared in this scope
http://hg.mozilla.org/mozilla-central/rev/3d8b07cdee97
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
It appears NativeMIPS.cpp and NativeSparc.cpp still need work.

Looking for help from Leon Sha on the sparc issue and Chris Dearman for mips.
Depends on: 551165
Looks like NativeSPARC.cpp was fixed via bug# 547232
Entered a new MIPS tracking bug as #551165
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.