504507 - nanojit: kill LIR64

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

I contend that LIR64 obfuscates nanojit's code more than it helps it.

I think the motivation was that you have various 32-bit/64-bit pairs of instructions and it made building and detecting those pairs easy -- eg. you can strip off the LIR64 part and just check if it's a LIR_call, or whatever.

But as part of the Great Opcode Renaming (bug 504506) opcodes now tend to come in trios:  i/q/f.  Which makes LIR64 actively dangerous.  Eg. look at this function in the nj-merge repo which uses a combination of LIR64 stripping and vanilla comparisons:

       bool isCall() const {
            LOpcode op = opcode();
            return (op & ~LIR64) == LIR_icall || op == LIR_qcall;
        }

Yuk.  Getting rid of it will also neaten up LIRopcode.tbl a bit.

Before I go ahead and code up a patch for this, I thought I'd wait and see if anyone has strong objections or if there is anything else I might be missing.

Comment 1

[Andreas Gal :gal]

The major uses for LIR64 are:

- quickly check if something is a quad. we can use a bitfield for this, and split for float and quad if thats desired (is it? I thought adobe prefers sticking with i/q split, and f being part of q).
- quickly translated from fop to iops. we can use a table for this.

This will greatly simplify the process of adding new opcodes to LIRopcode.tbl, which currently is a total pain.

++ from me for this. I doubt it will slow us down.

Comment 2

[Edwin Smith]

+1
I think we need a 128-entry attribute table with bits for quad, float, cond, comp, etc, operand counts, LIR sizes, LIR names (debug), etc.  similar to CallInfo

Comment 3

[Nicholas Nethercote [inactive]]

We have various tables (operandCount, repKind, etc.) that are constructed from the LIRopcode.tbl... are you happy with that approach?  Using the preprocessor has some uses, eg. the repKind is given as eg. "Op2" and that can be preprocessed into LRK_Op2 (an enum value) and also LInsOp2 (a class name).  I guess we could keep LIRopcode.tbl but only build one table for it instead of several tables.

isCseable is another attribute of interest.

Also, with LIR64 gone we shouldn't need to have both OPDEF and OPDEF64.

Comment 4

[Edwin Smith]

yes, i like the approach.  I was thinking one table vs N (as N gets larger), but its not a very important difference.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: NJ patch

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: TM patch

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: TR patch

Comment 8

[Nicholas Nethercote [inactive]]

These patches:

- Remove all traces of LIR64.  The 0..63 vs 64..127 distinction of opcodes
  is no longer meaningful, which is good, esp. since some of the opcodes
  higher in the range 64--127 aren't really 64-bit (eg. LIR_file,
  LIR_xbarrier).

- Add f64arithOp_to_i32arithOp() and i32cmpOp_to_i64cmpOp(), which replaces
  some cases where LIR64 was masked out.

- Add the retType field to LIRopcodes.tbl, which replaces some other cases
  where LIR64 was used to distinguish 32-bit from 64-bit opcodes.  This
  required adding the LTy type, which is based on the type of the same name
  in the LIR type-checking patch (bug 463137).

- Rename LInsI64 as LInsN64 because it's used for both 64-bit ints and
  64-bit floats, so "I64" was misleading.  It also overlapped with LTy_I64.

- Use isop() more in LIR functions like isCond() and isStore().

Comment 9

[Andreas Gal :gal]

Comment on attachment 416376 [details] [diff] [review]
NJ patch

 >+    const LTy retTypes[] = {
>+#define OPDEF(op, number, repKind, retType) \
>+        LTy_##retType,
>+#include "LIRopcode.tbl"
>+#undef OPDEF
>+        LTy_Void
>+    };
>+

LTy_Void is a very awkward name. Any chance we can improve the naming scheme?

>+    LOpcode f64arithOp_to_i32arithOp(LOpcode op)
>+    {
>+        switch (op) {
>+        case LIR_fneg:  return LIR_neg;
>+        case LIR_fadd:  return LIR_add;
>+        case LIR_fsub:  return LIR_sub;
>+        case LIR_fmul:  return LIR_mul;
>+        default:        NanoAssert(0); return LIR_skip;
>+        }
>+    }

We could store the equivalent op in the table to save this lookup table here. What do you think?

>+
>+    LOpcode i32cmpOp_to_i64cmpOp(LOpcode op)
>+    {
>+        switch (op) {
>+        case LIR_eq:    return LIR_qeq;
>+        case LIR_lt:    return LIR_qlt;
>+        case LIR_gt:    return LIR_qgt;
>+        case LIR_le:    return LIR_qle;
>+        case LIR_ge:    return LIR_qge;
>+        case LIR_ult:   return LIR_qult;
>+        case LIR_ugt:   return LIR_qugt;
>+        case LIR_ule:   return LIR_qule;
>+        case LIR_uge:   return LIR_quge;
>+        default:        NanoAssert(0); return LIR_skip;
>+        }
>+    }
>+

Dito.

>+            //return isGuard() || isBranch() ||
>+            //       (isCall() && !isCse()) ||
>+            //       isStore() ||
>+            //       isop(LIR_label) || isop(LIR_live) || isop(LIR_flive) ||
>+            //       isop(LIR_regfence) ||
>+            //       isRet();
>+            return retTypes[opcode()] == LTy_Void || isCall() && !isCse();

I wonder whether we still need some of the isXxx() cases.

Is this patch going to break any backends? sparc? ppc? arm?

Comment 10

[Andreas Gal :gal]

Comment on attachment 416377 [details] [diff] [review]
TM patch

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -1927,17 +1927,17 @@ public:
>         } else if (ci == &js_DoubleToInt32_ci) {
>             LIns* s0 = args[0];
>             if (s0->isconstf())
>                 return out->insImm(js_DoubleToECMAInt32(s0->imm64f()));
>             if (s0->isop(LIR_fadd) || s0->isop(LIR_fsub)) {
>                 LIns* lhs = s0->oprnd1();
>                 LIns* rhs = s0->oprnd2();
>                 if (isPromote(lhs) && isPromote(rhs)) {
>-                    LOpcode op = LOpcode(s0->opcode() & ~LIR64);
>+                    LOpcode op = f64arithOp_to_i32arithOp(s0->opcode());

That is the single worst function name you came up with so far ;)

How about s0->toIntOp() and s0->toFloatOp() or something like that? Its a bit asymmetrical I admit. Just an idea. Maybe you find something better than the name above.

Very nice patch otherwise (r+ either way, with a renaming at your leisure).

>                     return out->ins2(op, demote(out, lhs), demote(out, rhs));
>                 }
>             }
>             if (isi2f(s0) || isu2f(s0))
>                 return iu2fArg(s0);
> 
>             // XXX ARM -- check for qjoin(call(UnboxDouble),call(UnboxDouble))
>             if (s0->isCall()) {
>@@ -8245,17 +8245,17 @@ TraceRecorder::alu(LOpcode v, jsdouble v
>          */
>         guard(false, lir->ins2i(LIR_lt, d0, 0), exit);
>         branch->setTarget(lir->ins0(LIR_label));
>         break;
>       }
> #endif
> 
>       default:
>-        v = (LOpcode)((int)v & ~LIR64);
>+        v = f64arithOp_to_i32arithOp(v);
>         result = lir->ins2(v, d0, d1);
> 
>         /*
>          * If the operands guarantee that the result will be an integer (i.e.
>          * z = x + y with 0 <= (x|y) <= 0xffff guarantees z <= fffe0001), we
>          * don't have to guard against an overflow. Otherwise we emit a guard
>          * that will inform the oracle and cause a non-demoted trace to be
>          * attached that uses floating-point math for this operation.
>@@ -8932,17 +8932,17 @@ TraceRecorder::relational(LOpcode op, bo
> 
>     return ARECORD_CONTINUE;
> }
> 
> JS_REQUIRES_STACK RecordingStatus
> TraceRecorder::unary(LOpcode op)
> {
>     jsval& v = stackval(-1);
>-    bool intop = !(op & LIR64);
>+    bool intop = retTypes[op] == LTy_I32;
>     if (isNumber(v)) {
>         LIns* a = get(&v);
>         if (intop)
>             a = f2i(a);
>         a = lir->ins1(op, a);
>         if (intop)
>             a = lir->ins1(LIR_i2f, a);
>         set(&v, a);
>@@ -8965,17 +8965,17 @@ TraceRecorder::binary(LOpcode op)
>         }
>         return call_imacro(binary_imacros.obj_any);
>     }
>     if (!JSVAL_IS_PRIMITIVE(r)) {
>         RETURN_IF_XML(r);
>         return call_imacro(binary_imacros.any_obj);
>     }
> 
>-    bool intop = !(op & LIR64);
>+    bool intop = retTypes[op] == LTy_I32;
>     LIns* a = get(&l);
>     LIns* b = get(&r);
> 
>     bool leftIsNumber = isNumber(l);
>     jsdouble lnum = leftIsNumber ? asNumber(l) : 0;
> 
>     bool rightIsNumber = isNumber(r);
>     jsdouble rnum = rightIsNumber ? asNumber(r) : 0;
>diff --git a/js/src/lirasm/lirasm.cpp b/js/src/lirasm/lirasm.cpp
>--- a/js/src/lirasm/lirasm.cpp
>+++ b/js/src/lirasm/lirasm.cpp
>@@ -1743,23 +1743,20 @@ Lirasm::Lirasm(bool verbose) :
>     if (mVerbose) {
>         mLogc.lcbits = LC_Assembly | LC_RegAlloc | LC_Activation;
>         mLabelMap = new (mAlloc) LabelMap(mAlloc, &mLogc);
>         mLirbuf->names = new (mAlloc) LirNameMap(mAlloc, mLabelMap);
>     }
> #endif
> 
>     // Populate the mOpMap table.
>-#define OPDEF(op, number, repkind) \
>-    mOpMap[#op] = LIR_##op;
>-#define OPD64(op, number, repkind) \
>+#define OPDEF(op, number, repKind, retType) \
>     mOpMap[#op] = LIR_##op;
> #include "nanojit/LIRopcode.tbl"
> #undef OPDEF
>-#undef OPD64
> 
>     // TODO - These should alias to the appropriate platform-specific LIR opcode.
>     mOpMap["alloc"] = mOpMap["ialloc"];
>     mOpMap["param"] = mOpMap["iparam"];
> }
> 
> Lirasm::~Lirasm()
> {

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> 
> LTy_Void is a very awkward name. Any chance we can improve the naming scheme?

Alright, you've forced my hand:  LTy_Top (the proper name from a type theory POV) it is!  (That's the last time I try to make things easy for C programmers.)  And it foreshadows the type lattice that will eventually be added as part of bug 463137).

> >+    LOpcode f64arithOp_to_i32arithOp(LOpcode op)
> >+    {
> >+        switch (op) {
> >+        case LIR_fneg:  return LIR_neg;
> >+        case LIR_fadd:  return LIR_add;
> >+        case LIR_fsub:  return LIR_sub;
> >+        case LIR_fmul:  return LIR_mul;
> >+        default:        NanoAssert(0); return LIR_skip;
> >+        }
> >+    }
> 
> We could store the equivalent op in the table to save this lookup table here.
> What do you think?

In LIRopcode.tbl?  I think it's a bad idea -- it would require an extra field that is only relevant for four opcodes, and wouldn't speed things up because this function is called rarely.  (I did timings, the patch is perf-neutral.)


> >+            //return isGuard() || isBranch() ||
> >+            //       (isCall() && !isCse()) ||
> >+            //       isStore() ||
> >+            //       isop(LIR_label) || isop(LIR_live) || isop(LIR_flive) ||
> >+            //       isop(LIR_regfence) ||
> >+            //       isRet();
> >+            return retTypes[opcode()] == LTy_Void || isCall() && !isCse();
> 
> I wonder whether we still need some of the isXxx() cases.

Oh, that wasn't supposed to go in this patch, I meant to save that for bug 503990.  I'll take it out, I haven't checked/tested that code carefully yet.


> Is this patch going to break any backends? sparc? ppc? arm?

I haven't tested them but it shouldn't, X64 was the only backend to use LIR64 directly.


LOpcode(s0->opcode() & ~LIR64);
> >+                    LOpcode op = f64arithOp_to_i32arithOp(s0->opcode());
> 
> That is the single worst function name you came up with so far ;)
> 
> How about s0->toIntOp() and s0->toFloatOp() or something like that? Its a bit
> asymmetrical I admit. Just an idea. Maybe you find something better than the
> name above.

Ok, I'll do something shorter.  I figure long ugly names are ok for rarely-used functions, ie. Huffman encoding.

Comment 12

[Andreas Gal :gal]

I mean LTY_. Void or top is the same to me :)

Comment 13

[Nicholas Nethercote [inactive]]

It's similar to LIR_xyz :)

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: NJ patch v2

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: TM patch v2

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: TR patch v2

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 416816 [details] [diff] [review]
NJ patch v2

r=gal (he told me in person).

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 416817 [details] [diff] [review]
TM patch v2

r=gal (he told me in person).

Comment 19

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/2271d9cb2d87
http://hg.mozilla.org/tracemonkey/rev/11979742e2ae
http://hg.mozilla.org/tracemonkey/rev/d3ddc3edce12

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0793ed51d00c

Comment 21

[Nicholas Nethercote [inactive]]

Follow-up to fix two incorrect retTypes:

http://hg.mozilla.org/projects/nanojit-central/rev/52667ba8b5fe

Comment 22

[Nicholas Nethercote [inactive]]

Merge the follow-up to TM:

http://hg.mozilla.org/tracemonkey/rev/2dae4e3f9718