513844 - TM: Fix up tracer for x64 compatibility

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

We have a lot of places that use LIR_eq where it should be the new LIR_peq. This patch fixes those.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch v2

This version handles JSVAL_SPECIAL a little differently in unbox_jsval. We need to chop off the high bits I think.

Comment 2

[Jeff Walden [:Waldo]]

TraceRecorder::equalityHelper?

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: new version

This patch introduces a new SanityFilter, debug only, which type checks LIR. With this I have corrected every 64-bit type issue exposed by trace-tests, and I'm down to only 6 tests out of 306 failing.

Please double check that I've got things like the dense array index and string length stuff correct. This patch assumes we can always truncate those to 32 bits (see bug 513348).

Comment 4

[Andreas Gal :gal]

Comment on attachment 398070 [details] [diff] [review]
new version


>+              /* TM specific hack: see bug 503427 */
>+              JS_ASSERT_IF(s0->opcode() != LIR_i2f, !s0->isQuad());

r- for that. We have to fix the underlying bug first.

>+            default:
>+              break;
>+        }
>+        return out->ins1(v, s0);
>+    }

The filter should go into NJ I think.



> JS_REQUIRES_STACK JSRecordingStatus
> TraceRecorder::equalityHelper(jsval l, jsval r, LIns* l_ins, LIns* r_ins,
>                               bool negate, bool tryBranchAfterCond,
>                               jsval& rval)
> {
>-    bool fp = false;
>+    LOpcode op = LIR_eq;

Nice. Much clearer.


> JS_REQUIRES_STACK LIns*
> TraceRecorder::unbox_jsval(jsval v, LIns* v_ins, VMSideExit* exit)
> {
>     if (isNumber(v)) {
>         // JSVAL_IS_NUMBER(v)
>         guard(false,
>-              lir->ins_eq0(lir->ins2(LIR_pior,
>-                                     lir->ins2(LIR_piand, v_ins, INS_CONST(JSVAL_INT)),
>-                                     lir->ins2i(LIR_eq,
>-                                                lir->ins2(LIR_piand, v_ins,
>-                                                          INS_CONST(JSVAL_TAGMASK)),
>-                                                JSVAL_DOUBLE))),
>+              lir->ins_eq0(lir->ins2(LIR_or,
>+                                     truncateNativeUint(lir, lir->ins2(LIR_piand, v_ins,
>+                                                        INS_CONSTWORD(JSVAL_INT))),
>+                                     lir->ins2(LIR_peq,
>+                                               lir->ins2(LIR_piand, v_ins,
>+                                                         INS_CONSTWORD(JSVAL_TAGMASK)),
>+                                               INS_CONSTWORD(JSVAL_DOUBLE)))),


Why truncateNativeUint here? (why Uint and not int).


>-        return lir->ins2i(LIR_ush, v_ins, JSVAL_TAGBITS);
>+        return truncateNativeUint(lir, lir->ins2i(LIR_pursh, v_ins, JSVAL_TAGBITS));

Again why Unit?


>                                                   len_ins,
>                                                   INS_CONSTWORD(JSString::DEPENDENT_LENGTH_MASK)),
>                                         masked_len_ins));
>+    return truncateNativeUint(lir, real_len);

This one looks right.

>             guard(true,
>                   lir->ins2i(LIR_eq,
>-                             stobj_get_fslot(aobj_ins, JSSLOT_ARRAY_LENGTH),
>+                             truncateNativeUint(lir, stobj_get_fslot(aobj_ins,
>+                                                                     JSSLOT_ARRAY_LENGTH)),
>                              length),
>                   BRANCH_EXIT);

That looks ok too.


>             if (*pc == JSOP_NEW) {
>-                LIns* x = lir->ins_eq0(lir->ins2i(LIR_piand, v_ins, JSVAL_TAGMASK));
>-                x = lir->ins_choose(x, v_ins, INS_CONST(0));
>+                LIns* x = lir->ins_eq0(lir->ins2(LIR_piand, v_ins, INS_CONSTWORD(JSVAL_TAGMASK)));
>+                x = lir->ins_choose(x, v_ins, INS_CONSTWORD(0));

0?


>         } else {
>             if (!guardClass(obj, obj_ins, &js_SlowArrayClass, snapshot(BRANCH_EXIT)))
>                 ABORT_TRACE("can't trace length property access on non-array");
>         }
>-        v_ins = lir->ins1(LIR_i2f, stobj_get_fslot(obj_ins, JSSLOT_ARRAY_LENGTH));
>+        v_ins = lir->ins1(LIR_i2f,
>+                          truncateNativeInt(lir,
>+                                            stobj_get_fslot(obj_ins, JSSLOT_ARRAY_LENGTH)));

Now shouldn't this be Uint?

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #4)
> r- for that. We have to fix the underlying bug first.

I don't think so, but we certainly can. It's a temporary glitch that gets corrected as LIR passes through the pipeline.

> The filter should go into NJ I think.

Yes, agree.

> Why truncateNativeUint here? (why Uint and not int).

They are really the same thing, I should just call it "truncateTo32" or something. LIR_qlo doesn't say anything about the sign whereas LIR_i2q/LIR_u2q do.

Comment 6

[Andreas Gal :gal]

Ok, didn't know its fixed in the end. I was worried about i2f i2f down in the assembler. I take that back then. Yeah, drop the U then.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: use peq0 instead of eq0, rename truncation helper

Some of this isn't as pretty as it probably could be, so suggestions welcome.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 398501 [details] [diff] [review]
use peq0 instead of eq0, rename truncation helper

New patch coming soon.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: version something

The suspicious looking change in NativeX64.cpp is because TraceMonkey can write over valid jumps, when correcting guards. So that assertion is bogus when LIR_xanything is involved.

Comment 10

[Andreas Gal :gal]

Comment on attachment 399768 [details] [diff] [review]
version something


>+#define JSVAL_TAGMASK           ((jsval)JS_BITMASK(JSVAL_TAGBITS))

jsval(JS_BITMASK(JSVAL_TAGBITS))

>+#if defined NANOJIT_64BIT
>+#define PAGEMASK 0x7ff
>+#else
>+#define PAGEMASK 0xfff
>+#endif
>+

(NJ_PAGE_SIZE / sizeof(void*) * 4) - 1

> Tracker::Tracker()
> {
>     pagelist = 0;
> }
> 
> Tracker::~Tracker()
> {
>     clear();
> }
> 
> jsuword
> Tracker::getPageBase(const void* v) const
> {
>-    return jsuword(v) & ~jsuword(NJ_PAGE_SIZE-1);
>+    /*
>+     * NB: Must use PAGEMASK because on 64-bit platforms, less pointers can be stored in the map.
>+     */

Move this up to the PAGEMASK definition.

>+    return jsuword(v) & ~jsuword(PAGEMASK);
> }


> 
> 
>+inline LIns*
>+TraceRecorder::truncateTo32(nanojit::LIns* ins)
>+{
>+#ifdef NANOJIT_64BIT
>+    return lir->ins1(LIR_qlo, ins);
>+#else
>+    return ins;
>+#endif
>+}
>+

p2i


>     if (!cond->isCond()) {
>         expected = !expected;
>-        cond = lir->ins_eq0(cond);
>+        cond = cond->isQuad() ? lir->ins_peq0(cond) : lir->ins_eq0(cond);

Maybe we should teach the ins_eq0 helper to do ->isQuad() itself?

>--- a/js/src/nanojit/NativeX64.cpp
>+++ b/js/src/nanojit/NativeX64.cpp
>@@ -1237,17 +1237,17 @@ namespace nanojit
>             next = patch+5;
>         } else if (patch[0] == 0x0F && (patch[1] & 0xF0) == 0x80) {
>             // jcc disp32
>             next = patch+6;
>         } else {
>             next = 0;
>             TODO(unknown_patch);
>         }
>-        NanoAssert(((int32_t*)next)[-1] == 0);
>+        //NanoAssert(((int32_t*)next)[-1] == 0);

Remove assert and replace with a comment why.

>         NanoAssert(isS32(target - next));
>         ((int32_t*)next)[-1] = int32_t(target - next);
>         if (next[0] == 0x0F && next[1] == 0x8A) {
>             // code is jne<target>,jp<target>, for LIR_jf(feq)
>             // we just patched the jne, now patch the jp.
>             next += 6;
>             NanoAssert(((int32_t*)next)[-1] == 0);
>             NanoAssert(isS32(target - next));

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (From update of attachment 399768 [details] [diff] [review])
> 
> >+#define JSVAL_TAGMASK           ((jsval)JS_BITMASK(JSVAL_TAGBITS))
> 
> jsval(JS_BITMASK(JSVAL_TAGBITS))

No good in a C .h file like jsapi.h (it's one of the few that stay C, with JS_{BEGIN,END}_EXTERN_C, etc.).

> >+#if defined NANOJIT_64BIT
> >+#define PAGEMASK 0x7ff
> >+#else
> >+#define PAGEMASK 0xfff
> >+#endif
> >+
> 
> (NJ_PAGE_SIZE / sizeof(void*) * 4) - 1

4096 / sizeof(void*) is 1024 or 512, depending on target. But the * 4 applies to the result of the division, and shifts 1024 or 512 left by 2, so -1 borrows a couple of zeros only. Is this really right?

/be

Comment 12

[Brendan Eich [:brendan]]

Sorry, I was unclear, or just wrong -- but shift left by 2 still wins, esp. with a comment on why. The magic 4 just invites confusion.

/be

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v6

Adds a comment to PAGEMASK and makes the expression less magical.
Also fixed another pointer bug in TraceRecorder::newArguments.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 399813 [details] [diff] [review]
v6

>+#define JSVAL_TAGMASK           ((jsval)JS_BITMASK(JSVAL_TAGBITS))

Let that breathe a bit: ((jsval) JS_BITMASK(...)).

>+ * This confusing and mysterious expression is used for the Tracker. The
>+ * tracker's responsibility is to map opaque, 4-byte aligned adresses to LIns

"addresses"

/be

Comment 15

[Mike Shaver (:shaver emeritus)]

(jsval(JS_BITMASK(...)))

might just be me, but when it's all nested "one way" like that I find I don't need the spacing to help me split it out.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/6446f2e2e07b

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/6b2fb36f76e3

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/f3351b36fd5b

Comment 19

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d1c35813e796