533659 - tracing support for JS webgl types

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

bug 532774 adds support for the WebGL array types to JS, this bug should teach the tracer how to generate optimized LIR for get/set on these.  There are 7 types that are necessary here:

int32
int16
int8
uint32
uint16
uint8
float

We already have support for reading uint32 and uint16 (ldc and ldcs, both zero-extend), and for writing 32-bit values.  We need to add the missing ones, including sign-extended 16-bit and 8-bit reads and 16-bit/8-bit writes.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: WIP, v0

Here's a work-in-progress attempt -- adds tracing for get element.  Adds some of the other missing lir ops as well, though doesn't use some of them yet.

Comment 2

[Andreas Gal :gal]

Comment on attachment 416719 [details] [diff] [review]
WIP, v0


>+    /* XXX are we supposed to be reading undefined here? */
>+

Lets always return a 0 here (can we change the spec?). That would help a lot avoiding type-instability.

>+    /* Ensure idx >= 0 */
>+    guard(true,
>+          lir->ins2i(LIR_ge, idx_ins, 0),
>+          exit);
>+

JSPackedArrayTypeEnum::FOO maybe?


>+                case LIR_ldcb_signed:
>+                case LIR_ldcs_signed:

ldcb is constant byte load. Is that what we want? I think we want ldb and ldbs.

>+                case LIR_fs2fd:
>+                {
>+                    countlir_fpu();
>+                    asm_fs2fd(ins);
>+                    break;
>+                }
>+                case LIR_fd2fs:
>+                {
>+                    countlir_fpu();
>+                    asm_fd2fs(ins);
>+                    break;
>+                }

I think Adobe calls these f32 (yeah, I know ...).

>+                case LIR_stcb:
>+                {

store constant byte? I don't think so.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: trace typed arrays

Working patch

Comment 4

[Andreas Gal :gal]

jstypedarray.h is missing

Comment 5

[Andreas Gal :gal]

ok found it

Comment 6

[Andreas Gal :gal]

Lets talk about the read-beyond-length case in person, the rest looks good.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch

updated based on review feedback

Comment 8

[Andreas Gal :gal]

Comment on attachment 423386 [details] [diff] [review]
updated patch

>b=533659; tracing support for JS typed array types; r=gal
>
>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -75,6 +75,7 @@
> #include "jsstaticcheck.h"
> #include "jstracer.h"
> #include "jsxml.h"
>+#include "jstypedarray.h"
> 
> #include "jsatominlines.h"
> #include "jsobjinlines.h"
>@@ -11952,6 +11953,15 @@ TraceRecorder::getPropertyWithNativeGett
>     return RECORD_CONTINUE;
> }
> 
>+#ifdef NANOJIT_IA32
>+// Typed array tracing is only on IA32 temporarily -- depends
>+// on NJ_EXPANDED_LOADSTORE and LIR_f2i
>+static inline bool OkToTraceTypedArrays() { return true; }
>+#else
>+static inline bool OkToTraceTypedArrays() { return false; }
>+#endif

You could also just have used a static const bool flag.

>+        if (isNumber(v)) {
>+            if (isPromoteInt(v_ins) &&
>+                tarray->type != js::TypedArray::TYPE_FLOAT32)
>+            {

We put the brace at the end of the line here I think.

>+                LIns *v_ins_int = demote(lir, v_ins);
>+                switch (tarray->type) {
>+                  case js::TypedArray::TYPE_INT8:
>+                  case js::TypedArray::TYPE_UINT8:
>+                    addr_ins = lir->ins2(LIR_piadd, data_ins, pidx_ins);
>+                    lir->insStore(LIR_stb, v_ins_int, addr_ins, 0);
>+                    break;
>+                  case js::TypedArray::TYPE_INT16:
>+                  case js::TypedArray::TYPE_UINT16:
>+                    addr_ins = lir->ins2(LIR_piadd, data_ins, lir->ins2i(LIR_pilsh, pidx_ins, 1));
>+                    lir->insStore(LIR_sts, v_ins_int, addr_ins, 0);
>+                    break;
>+                  case js::TypedArray::TYPE_INT32:
>+                  case js::TypedArray::TYPE_UINT32:
>+                    addr_ins = lir->ins2(LIR_piadd, data_ins, lir->ins2i(LIR_pilsh, pidx_ins, 2));
>+                    lir->insStore(LIR_sti, v_ins_int, addr_ins, 0);
>+                    break;
>+                  default:
>+                    JS_NOT_REACHED("Unknown typed array in tracer");
>+                }
>+            } else {
>+                switch (tarray->type) {
>+                  case js::TypedArray::TYPE_INT8:
>+                  case js::TypedArray::TYPE_UINT8:
>+                    addr_ins = lir->ins2(LIR_piadd, data_ins, pidx_ins);
>+                    lir->insStore(LIR_stb, lir->ins1(LIR_f2i, v_ins), addr_ins, 0);
>+                    break;

All of this could be simplified quite a bit.

if (type == FLOAST32) {
   float_code;
   return;
}

if (!isPromoteInt(v_ins))
    v_ins = lir->ins1(LIR_f2i, v_ins);

size = opcode_table_mapping_typed_array_enum_to_element_width[type];
addr_ins = lir->ins2(LIR_piadd,
                     data_ins,
                     size == 1 ? pidx_ins : lir->ins2i(LIR_pilsh, pidx_ins, size))
lir->insStore(opcode_table_mapping_typed_array_enum_to_LIR_opcode[ty]e, v_ins, addr_ins)

If you want to do this I can review again. If you want to stick to the current code, I think I can live with that.

>+    /*
>+     * Ensure idx < length
>+     *
>+     * NOTE! mLength is uint32, but it's guaranteed to fit in a jsval
>+     * int, so we can treat it as either signed or unsigned.
>+     * If the index happens to be negative, when it's treated as
>+     * unsigned it'll be a very large int, and thus won't be less than
>+     * length.
>+     */

Can we make valid super large (31-bit large) arrays on 64-bit machines?

>+    switch (tarray->type) {
>+      case js::TypedArray::TYPE_INT8:
>+        addr_ins = lir->ins2(LIR_piadd, data_ins, pidx_ins);
>+        v_ins = lir->ins1(LIR_i2f, lir->insLoad(LIR_ldsb, addr_ins, 0));
>+        break;
>+      case js::TypedArray::TYPE_UINT8:
>+        addr_ins = lir->ins2(LIR_piadd, data_ins, pidx_ins);
>+        v_ins = lir->ins1(LIR_u2f, lir->insLoad(LIR_ldzb, addr_ins, 0));
>+        break;
>+      case js::TypedArray::TYPE_INT16:
>+        addr_ins = lir->ins2(LIR_piadd, data_ins, lir->ins2i(LIR_pilsh, pidx_ins, 1));
>+        v_ins = lir->ins1(LIR_i2f, lir->insLoad(LIR_ldss, addr_ins, 0));
>+        break;
>+      case js::TypedArray::TYPE_UINT16:
>+        addr_ins = lir->ins2(LIR_piadd, data_ins, lir->ins2i(LIR_pilsh, pidx_ins, 1));
>+        v_ins = lir->ins1(LIR_u2f, lir->insLoad(LIR_ldzs, addr_ins, 0));
>+        break;
>+      case js::TypedArray::TYPE_INT32:
>+        addr_ins = lir->ins2(LIR_piadd, data_ins, lir->ins2i(LIR_pilsh, pidx_ins, 2));
>+        v_ins = lir->ins1(LIR_i2f, lir->insLoad(LIR_ld, addr_ins, 0));
>+        break;
>+      case js::TypedArray::TYPE_UINT32:
>+        addr_ins = lir->ins2(LIR_piadd, data_ins, lir->ins2i(LIR_pilsh, pidx_ins, 2));
>+        v_ins = lir->ins1(LIR_u2f, lir->insLoad(LIR_ld, addr_ins, 0));
>+        break;

Same here. This could be a lot shorter if you want to.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/tracemonkey/rev/df882f68ed0c

Didn't shorten (though started to) -- the shortening won't work with the magic clamped uint8 stuff in bug 534467, and so the code would just end up harder to follow.  Fixed the other nits, though.