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.

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.

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.

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

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.

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"...

(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...

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.

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.

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.

(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

A minor follow-up fix: http://hg.mozilla.org/projects/nanojit-central/rev/ac0fcaac8b16

NativePPC.cpp needed lots of love and was missing it, both 32 and 64 bit. (This is additive to the previous patch)

http://hg.mozilla.org/tamarin-redux/rev/ec68add0be82 http://hg.mozilla.org/tamarin-redux/rev/b8f64e82da3f

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

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.

Looks like NativeSPARC.cpp was fixed via bug# 547232 Entered a new MIPS tracking bug as #551165