Last Comment Bug 616169 - Update the LIR opcode table file format to make adding opcodes easier
: Update the LIR opcode table file format to make adding opcodes easier
Status: RESOLVED FIXED
fixed-in-nanojit, fixed-in-tamarin
:
Product: Core Graveyard
Classification: Graveyard
Component: Nanojit (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: float&float4_JIT
  Show dependency treegraph
 
Reported: 2010-12-02 09:37 PST by Virgil Palanciuc
Modified: 2014-03-17 08:00 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (32.55 KB, patch)
2010-12-02 09:40 PST, Virgil Palanciuc
n.nethercote: feedback+
Details | Diff | Review
patch (31.99 KB, patch)
2010-12-08 00:52 PST, Virgil Palanciuc
edwsmith: review+
Details | Diff | Review
Update the LIR opcode table file format (31.99 KB, text/plain)
2011-05-17 02:48 PDT, Virgil Palanciuc
no flags Details
Update the LIR opcode table file format (31.99 KB, patch)
2011-05-17 02:50 PDT, Virgil Palanciuc
no flags Details | Diff | Review
small lirasm change to compile with new .tbl format (858 bytes, patch)
2011-05-19 02:04 PDT, Virgil Palanciuc
no flags Details | Diff | Review
Update the LIR opcode table file format (32.21 KB, patch)
2011-10-12 00:32 PDT, Virgil Palanciuc
no flags Details | Diff | Review

Description Virgil Palanciuc 2010-12-02 09:37:30 PST
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 2010-12-02 09:40:52 PST
Created attachment 494740 [details] [diff] [review]
patch
Comment 2 Edwin Smith 2010-12-07 13:41:40 PST
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 [:njn] 2010-12-07 14:09:51 PST
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 [:njn] 2010-12-07 14:13:40 PST
(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 2010-12-07 17:18:22 PST
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 2010-12-08 00:51:38 PST
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 2010-12-08 00:52:02 PST
Created attachment 496099 [details] [diff] [review]
patch
Comment 8 Virgil Palanciuc 2010-12-08 02:21:31 PST
(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 [:njn] 2010-12-08 14:10:25 PST
Is it worth statically asserting that LIR_start == 0?  And LIR_sentinel < 256?  That's better than nothing.
Comment 10 Edwin Smith 2010-12-13 07:09:44 PST
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 2011-05-17 02:48:25 PDT
Created attachment 532908 [details]
Update the LIR opcode table file format
Comment 12 Virgil Palanciuc 2011-05-17 02:50:25 PDT
Created attachment 532909 [details] [diff] [review]
Update the LIR opcode table file format

re-uploaded, marked attachment as "patch"
Comment 13 Virgil Palanciuc 2011-05-19 02:04:43 PDT
Created attachment 533564 [details] [diff] [review]
small lirasm change to compile with new .tbl format
Comment 14 Edwin Smith 2011-10-04 07:43:24 PDT
Assigned to Rick to land.
Comment 15 Rick Reitmaier 2011-10-11 21:48:55 PDT
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 2011-10-12 00:32:49 PDT
Created attachment 566455 [details] [diff] [review]
Update the LIR opcode table file format
Comment 17 Virgil Palanciuc 2011-10-12 00:35:43 PDT
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 20 Tamarin Bot 2011-10-13 10:55:10 PDT
changeset: 6640:225bfd318eb7
user:      Virgil Palanciuc <virgilp@adobe.com>
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 2011-10-20 08:49:52 PDT
has landed in tamarin-redux
Comment 22 Edwin Smith 2011-10-20 09:08:00 PDT
Just following nanojit-central protocol; leaving upen until fixed-in-tracemonkey
Comment 23 Edwin Smith 2012-01-11 07:45:41 PST
Closing on the assumption TM is no longer pulling from NC.

Note You need to log in before you can comment on or make changes to this bug.