616169 - Update the LIR opcode table file format to make adding opcodes easier

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Virgil Palanciuc]

User-Agent:       Jakarta Commons-HttpClient/3.0 (Deskzilla Lite/2.1.5508.129)
Build Identifier: 

In order to add new types to Tamarin, we need new instructions in the JIT. 
There were only 9 opcodes available; this patch does the following:
- changes LOpcode from "enum" to unsigned", and adds the members as a series of 
global constants (thus avoiding compiler variations on enum basetype)
- adds a few more asserts to make sure that the assumptions made on 
opcodes/instructions still hold
- removes the need for explicitly specifying the "opcode number" in the .tbl 
file.

Reproducible: Always

Comment 1

[Virgil Palanciuc]

Attachment: patch

Comment 2

[Edwin Smith]

Comment on attachment 494740 [details] [diff] [review]
patch

Nick - just looking for feedback at this point.  I wonder if the patch is simpler if we just change the opcode field to unsigned, and add casts to the LIns accessor functions only, to squash warnings and sign-extension bugs.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 494740 [details] [diff] [review]
patch

The basic idea looks fine.  Various nits below;  I approve the change once
they are fixed.


> namespace nanojit
> {
>-    enum LOpcode
>-#if defined(_MSC_VER) && _MSC_VER >= 1400
>-#pragma warning(disable:4480) // nonstandard extension used: specifying underlying type for enum
>-          : unsigned
>-#endif
>+    typedef unsigned LOpcode;

'uint32_t' is better than 'unsigned' here.


>+    enum ___LOpcode
>     {
>-#define OP___(op, number, repKind, retType, isCse) \
>-        LIR_##op = (number),
>+#define OP___(op, repKind, retType, isCse) \
>+        __initializer_for_LIR_##op,
> #include "LIRopcode.tbl"
>-        LIR_sentinel,
>+        __initializer_for_LIR_sentinel,
> #undef OP___
>+    };

Remove the ',' after the sentinel, having a comma on the last element of an
enum is supported by some compilers but is non-standard.


>+// check that all opcodes are consecutive  (i.e. there's no explicit value assignment that may mess-up the values)

This comment is > 100 chars long, please split over two lines.  Also,
capital letters and full stops are good things to have in sentences :)


>+extern void nano_static_assert(int arg[(0      ==
>+#define OP___(op, repKind, retType, isCse) \
>+                                LIR_##op)?1:-1]); \
>+NanoStaticAssert(LIR_##op>=0 && LIR_##op < 256);   \
>+extern void nano_static_assert(int arg[(LIR_##op+1 ==
>+#include "LIRopcode.tbl"
>+#undef OP___
>+                                LIR_sentinel)?1:-1]);

I understand the aim of this code, but I don't understand the code itself.
Can it be simplified?  Also, use whitespace consistently around operators,
please, eg. "op + 1", "op >= 0".


>+ * Definitions of LIR opcodes.  If you need to allocate an opcode, you can insert one anywhare, but mind the  comments,
>+ * some opcodes need to be in-sequence, some operands need to be "complements" and the first should start at an even 
>+ * position (such that  OPCODE1 ^1 == OPCODE2 - e.g. LIR_jt^1==LIR_jf. These cases are checked with static asserts in the code,
>+ * if such an assert fails, use OP_UN to create "gaps" in order to keep the first in pair aligined to an even value.

Remove the extra space in "the  comments".

No need for quotes around "complements" or "gaps".

No need for space in "OPCODE1 ^1".

s/aligined/aligned/


>+ * It is reccomended that you group together opcodes that have similar semantics (e.g. comparisons, arithmetic operations, etc.)

s/reccomended/recommended/


>  *   OP___: for opcodes supported on all platforms.
>- *   OP_UN: for opcodes not yet used on any platform.
>+ *   OP_UN: used to create a "gap" in the opcode space

No need for quotes around "gap".


> #ifdef NANOJIT_64BIT
>-#   define OP_32(a, b, c, d, e)     OP_UN(b)
>+#   define OP_32(a, c, d, e)     OP_UN(a)
> #   define OP_64                    OP___
> #else
> #   define OP_32                    OP___
>-#   define OP_64(a, b, c, d, e)     OP_UN(b)
>+#   define OP_64(a, c, d, e)     OP_UN(a)
> #endif

Please preserve the aligned indentation on the right-hand side.


> 
> #if NJ_SOFTFLOAT_SUPPORTED
> #   define OP_SF                    OP___
> #else
>-#   define OP_SF(a, b, c, d, e)     OP_UN(b)
>+#   define OP_SF(a, c, d, e)     OP_UN(a)
> #endif
> 
> #if defined NANOJIT_IA32 || defined NANOJIT_X64
> #   define OP_86                    OP___
> #else
>-#   define OP_86(a, b, c, d, e)     OP_UN(b)
>+#   define OP_86(a, c, d, e)     OP_UN(a)
> #endif

Ditto.

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> 
> Nick - just looking for feedback at this point.  I wonder if the patch is
> simpler if we just change the opcode field to unsigned, and add casts to the
> LIns accessor functions only, to squash warnings and sign-extension bugs.

The patch typdefs 'LOpcode' to 'unsigned' (but I recommended using 'uint32_t' instead), so there's no need to do this.

Comment 5

[Edwin Smith]

I was thinking we could keep LOpcode defined as the enum, but declare LIns.sharedFields.opcode as uint32_t:8.  That way we keep some of the
type checking we have for other locals and arguments declared as LOpcode, and I don't think we'd need as much, or any, pragma magic.

This is based on the assumption that all we'd need is casts between
uint32<-->LOpcode in accessor functions of LIns, but nowhere else.  If that's false, then either way the patch is big, and the current one is fine.

Comment 6

[Virgil Palanciuc]

I modified it to be an enum - but the sharedFields.opcode is not uint8_t (seemed strange to make it uint32_t:8 :) ). Also, did the changes suggested by NIck.

Comment 7

[Virgil Palanciuc]

Attachment: patch

Comment 8

[Virgil Palanciuc]

(In reply to comment #6)
> I modified it to be an enum - but the sharedFields.opcode is not uint8_t
> (seemed strange to make it uint32_t:8 :) ). Also, did the changes suggested by
> NIck.

I meant it is NOW uint8_t (instead of "it is not").

Also - to Nicks' remark that the static_asserts should be simplified: I removed them completely; there is actually no way now to actually assign an explicit value to any opcode from LIROpcodes.tbl; the only way those assertions could fail would be if we had a non-standard compiler that decides to do "creative" value assignment for enum members - but I don't think this will ever be the case.

Comment 9

[Nicholas Nethercote [inactive]]

Is it worth statically asserting that LIR_start == 0?  And LIR_sentinel < 256?  That's better than nothing.

Comment 10

[Edwin Smith]

Comment on attachment 496099 [details] [diff] [review]
patch

Nick's static assert suggestion is good, and I'd also add an assert
in initSharedFields() that ensures the opcode value isn't truncated.  Is it really the case that no compilers are complaining about assigning LOpcode to uint8_t without a cast?

below are cosmetic - please fix, but no need to re-review.  

LIR.cpp:98  the comment should read:

   // Check that all opcodes are between 0 and 255.
                             ^^^                  ^

LIR.h:669.  I trust compilers to pack uint32_t opcode:8 in with the other bits in the bit field, more than I trust compilers to pack a uint8_t in with the other fields.  But, the static asserts in staticSanityCheck() should fire if sizeof(SharedFields) != sizeof(void*), so I can live with it.

LIR.h: could you break LIns::opcode() into multiple lines?

Comment 11

[Virgil Palanciuc]

Attachment: Update the LIR opcode table file format

Comment 12

[Virgil Palanciuc]

Attachment: Update the LIR opcode table file format

re-uploaded, marked attachment as "patch"

Comment 13

[Virgil Palanciuc]

Attachment: small lirasm change to compile with new .tbl format

Comment 14

[Edwin Smith]

Assigned to Rick to land.

Comment 15

[Rick Reitmaier]

Virgil, I kicked off a sandbox build with this change and am seeing compile failures.  Just wondering how extensively this patch has been vetted.  Has it been compiled for all platforms/releases that we target?

It could be pilot error, as there were other changes that I landed in that build, but the errors (warnings actually) were appearing in/around LIR.h.

Comment 16

[Virgil Palanciuc]

Attachment: Update the LIR opcode table file format

Comment 17

[Virgil Palanciuc]

Sorry, I had the #pragma warning guarded by #ifdef MSC_VER in another patch from my queue.... I updated it now, and launched the buildbot, everything seems to work fine now.

Comment 18

[Rick Reitmaier]

virgilp http://hg.mozilla.org/projects/nanojit-central/rev/e93aaafc770a

Comment 19

[Rick Reitmaier]

virgilp http://hg.mozilla.org/projects/nanojit-central/rev/51503eeadedf

Comment 20

[Tamarin Bot]

changeset: 6640:225bfd318eb7
user:      Virgil Palanciuc <[e-mail]>
summary:   Bug 616169 - Update the LIR opcode table file format to make adding opcodes easier (r=edwsmith,nnethercote)

http://hg.mozilla.org/tamarin-redux/rev/225bfd318eb7

Comment 21

[Virgil Palanciuc]

has landed in tamarin-redux

Comment 22

[Edwin Smith]

Just following nanojit-central protocol; leaving upen until fixed-in-tracemonkey

Comment 23

[Edwin Smith]

Closing on the assumption TM is no longer pulling from NC.