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

RESOLVED FIXED

Status

Core Graveyard
Nanojit
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: Virgil Palanciuc, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
Blocks: 615871
(Reporter)

Comment 1

7 years ago
Created attachment 494740 [details] [diff] [review]
patch
(Reporter)

Updated

7 years ago
Attachment #494740 - Flags: review?(edwsmith)

Updated

7 years ago
Component: JIT Compiler (NanoJIT) → Nanojit
Product: Tamarin → Core
QA Contact: nanojit → nanojit

Comment 2

7 years ago
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.
Attachment #494740 - Flags: feedback?(nnethercote)
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.
Attachment #494740 - Flags: feedback?(nnethercote) → feedback+
(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

7 years ago
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.
(Reporter)

Comment 6

7 years ago
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.
(Reporter)

Comment 7

7 years ago
Created attachment 496099 [details] [diff] [review]
patch
(Reporter)

Updated

7 years ago
Attachment #494740 - Attachment is obsolete: true
Attachment #494740 - Flags: review?(edwsmith)
(Reporter)

Comment 8

7 years ago
(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.
Is it worth statically asserting that LIR_start == 0?  And LIR_sentinel < 256?  That's better than nothing.

Comment 10

7 years ago
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?

Updated

7 years ago
Attachment #496099 - Flags: review+
(Reporter)

Comment 11

6 years ago
Created attachment 532908 [details]
Update the LIR opcode table file format
Attachment #496099 - Attachment is obsolete: true
(Reporter)

Comment 12

6 years ago
Created attachment 532909 [details] [diff] [review]
Update the LIR opcode table file format

re-uploaded, marked attachment as "patch"
Attachment #532908 - Attachment is obsolete: true
(Reporter)

Comment 13

6 years ago
Created attachment 533564 [details] [diff] [review]
small lirasm change to compile with new .tbl format

Comment 14

6 years ago
Assigned to Rick to land.
Assignee: nobody → rreitmai
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 15

6 years ago
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.
(Reporter)

Comment 16

6 years ago
Created attachment 566455 [details] [diff] [review]
Update the LIR opcode table file format
Attachment #532909 - Attachment is obsolete: true
(Reporter)

Comment 17

6 years ago
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

6 years ago
virgilp http://hg.mozilla.org/projects/nanojit-central/rev/e93aaafc770a
Whiteboard: fixed-in-nanojit

Comment 19

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

Comment 20

6 years ago
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
(Reporter)

Comment 21

6 years ago
has landed in tamarin-redux
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 22

6 years ago
Just following nanojit-central protocol; leaving upen until fixed-in-tracemonkey
Assignee: rreitmai → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tamarin

Comment 23

5 years ago
Closing on the assumption TM is no longer pulling from NC.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.