Implement methodjit typed arrays

[GoogleCodeExporter]

POLYIC based typed arrays no longer function, so we have been working without 
typed array support. Based on BaseAssembler.h, we need

load8SignExtend
load8ZeroExtend
load16SignExtend
(load16)
(load32)

We have these already, so we should just turn it on and see what breaks.

This may not make 15 due to issue 165, but it must make 16.

Original issue reported on code.google.com by classi...@floodgap.com on 5 Jul 2012 at 1:27

• Blocked on: #165

[GoogleCodeExporter]

Original comment by classi...@floodgap.com on 5 Jul 2012 at 1:27

• Added labels: Milestone-Unstable
• Now blocked on: #165

[GoogleCodeExporter]

Those functions are indeed implemented but not with a BaseIndex as first 
argument.

Original comment by Tobias.N...@gmail.com on 5 Jul 2012 at 3:04

[GoogleCodeExporter]

Currently trying to implement the missing flavours of the above functions. 
load8 , loadFloat and loadDouble have to be added to the list. A first try is 
being built right now - I'll test that tomorrow.

Original comment by Tobias.N...@gmail.com on 5 Jul 2012 at 11:37

[GoogleCodeExporter]

Go for it! I'm still trying to get 15 to build, and then attack issue 166.

Original comment by classi...@floodgap.com on 6 Jul 2012 at 12:16

[GoogleCodeExporter]

convertDoubleToFloat() is also missing; should be a simple call to 
assembler.frsp().

Original comment by Tobias.N...@gmail.com on 6 Jul 2012 at 12:17

[GoogleCodeExporter]

fastLoadDouble() is also missing. AFAIK there's no way to do that thing any 
faster (moving two floats, high and low byte, from GPRs into one FPR) so I'll 
try to avoid the need for this function.

Original comment by Tobias.N...@gmail.com on 6 Jul 2012 at 1:38

[GoogleCodeExporter]

That might not be as easy... a couple of tests are failing (crashing mostly) 
although they don't use any of the functions I added.
The crashes I examined so far happen when GenConversionForFloatArray or 
GenConversionForIntArray try to store their values. It asserts in 
checkStackPointer() with "Assertion failure: a.offset >= 24".

Original comment by Tobias.N...@gmail.com on 6 Jul 2012 at 9:23

[GoogleCodeExporter]

That assertion means something is trying to write into the linkage area, which 
is bad(tm).

Original comment by classi...@floodgap.com on 6 Jul 2012 at 1:36

[GoogleCodeExporter]

The following lines in TypedArrayIC.h are causing problems:

        StackMarker vp = masm.allocStack(sizeof(Value), sizeof(double));
        masm.storeValue(vr, masm.addressOfExtra(vp));

It seems JaegerSpew() is to blame. Upon entering store32() the address.base is 
r1 and after the call to JaegerSpew() it is r0 (the addess.offset stays 0).

Original comment by Tobias.N...@gmail.com on 6 Jul 2012 at 4:41

[GoogleCodeExporter]

With JaegerSpew support disabled (release build, debug off) the crashes are 
gone.
Remaining some (non crashing) unexpected fails (5 so far in the first 70% of 
the tests).

Original comment by Tobias.N...@gmail.com on 6 Jul 2012 at 9:33

[GoogleCodeExporter]

The remaining fails are:
basic/testTypedArrayUint32.js
jaeger/normalIntTypedArrays.js
testSetTypedIntArray.js
basic/bug620532.js (failing with flags amn only)

The problem in these tests is with Uint32 arrays and the particular integer 
value of 4294967295 (0xFFFFFFFF). That value is correctly converted to a double 
value internally but after storing it into the typed array and loading that 
back in it is interpreted as -4503599627370496 (type inference off) or NaN 
(type inference on).

Original comment by Tobias.N...@gmail.com on 7 Jul 2012 at 11:20

[GoogleCodeExporter]

Some more investigation reveals the following behaviour:
- occurs for all values between 0x80000000 and 0xFFFFFFFF
- storing such values to a typed array works
  a) 1 time when type inference is on
  b) 2 times when type inference is off

Original comment by Tobias.N...@gmail.com on 7 Jul 2012 at 2:53

[GoogleCodeExporter]

a) invalid values are always NaN
b) first invalid value is between -4503599627370496 (for 0xFFFFFFFF) and 
-4503597479886848 (for 0x80000000), subsequent invalid values are NaN

Original comment by Tobias.N...@gmail.com on 7 Jul 2012 at 2:58

[GoogleCodeExporter]

So here's a patch that Works For Me (TM) -- it passes all the jit-tests with 
DEBUG enabled. It's probably not very different from your code, Tobias, but for 
two points:
1. the definition of BaseStackSpace in methodjit/BaseAssembler.h keeps the 
methodjit from trying to write to the linkage areas, and
2. the bugfix in convertUInt32ToDouble() fixes test failures just like the ones 
you saw.

The patch goes on top of the patch stack from bug 165 comment 38 and the patch 
from bug 165 comment 53.

Original comment by magef...@gmail.com on 8 Jul 2012 at 10:20

Attachments:

• 10.4Fx15-enable-typedarrays.patch

[GoogleCodeExporter]

Oh, and I should mention that the patch is relative to the js/ directory rather 
than the whole tree.

Original comment by magef...@gmail.com on 8 Jul 2012 at 10:22

[GoogleCodeExporter]

That looks good. I'm wondering if there's a better way to do storeDouble, but I 
can't think of one.

Original comment by classi...@floodgap.com on 8 Jul 2012 at 10:30

[GoogleCodeExporter]

I wonder if your change to convertInt32ToDouble should be ported to stable too. 
It shouldn't hurt, and might fix some edge cases.

Original comment by classi...@floodgap.com on 8 Jul 2012 at 10:35

[GoogleCodeExporter]

You mean the storeDouble(ImmDouble,...) version? The x86 assembler does 
basically the same thing I did, doing the store through GPRs rather than FPRs.

According to a quick grep, convertUInt32ToDouble (as opposed to 
convertInt32ToDouble) is only used by the typed array code.

Original comment by magef...@gmail.com on 8 Jul 2012 at 10:44

[GoogleCodeExporter]

Yes, but your patch is to convertInt32ToDouble. Did you mean to do that?

Original comment by classi...@floodgap.com on 8 Jul 2012 at 11:03

[GoogleCodeExporter]

Oops, never mind, I read the patch wrong. Duh.

Original comment by classi...@floodgap.com on 8 Jul 2012 at 11:06

[GoogleCodeExporter]

Ah, I see -- the patch is a bit confusing to look at, but there is a hunk break 
hiding in there (the line before the #endif at the start of the last hunk) 
which shows that the patch is actually in convertUInt32ToDouble.

Original comment by magef...@gmail.com on 8 Jul 2012 at 11:10

[GoogleCodeExporter]

My patch is indeed quite similar - but instead of copying existing code I 
preferred calling that code, which shouldn't hurt performance if those 
functions are all inlined.
I'll see if mine works with that and in case it does I'll post it here as well.

Original comment by Tobias.N...@gmail.com on 9 Jul 2012 at 1:41

[GoogleCodeExporter]

Calling rather than copying is generally a good practice, no argument! (In this 
case, though, most of the copying is to ensure that we don't do redundant 
address generation, such as would happen from e.g. calling store32() twice. 
That said, I strongly suspect that there are a bunch of places where the total 
LOC could be appreciably trimmed by using functions template<>ed on the address 
type, ImplicitAddress or BaseInddex.)

Original comment by magef...@gmail.com on 9 Jul 2012 at 3:03

[GoogleCodeExporter]

So now here my patch.
I didn't test with your patch (rebuilding takes so much time here) but with 
mine there are two interesting fails:
jaeger/testSetTypedIntArray.js
jaeger/testSetTypedFloatArray.js

Those two tests fail (with jitflags "am" only) with exit code 03 when the call 
to gc() is done after 5 loop iterations. It's gc() itself that causes the 
abortion. When I comment that call out the tests both pass.

Original comment by Tobias.N...@gmail.com on 9 Jul 2012 at 6:39

• Changed state: Started

Attachments:

• typedarray.patch

[GoogleCodeExporter]

The five loop iterations sounds like a stub getting promoted (see issue 165). 
Otherwise they look very similar. I'm trying to work out an issue with Aurora 
15 crashing at quit (I think it's a rogue ObjC instance that the runtime thinks 
is still active and tries to send a dealloc message) and then I'll land this 
while I still have a DEBUG build up to verify no issues. 15 is a real pain in 
the @$$ with all the widget changes; it took me most of yesterday to get it to 
even not crash just pressing a key.

Original comment by classi...@floodgap.com on 9 Jul 2012 at 1:20

[GoogleCodeExporter]

Well, at least in that typed array tests everything is done 10 times and after 
the fifth time the garbage collector is called.
When I change the moment the garbage collector is called to the first or second 
iteration both tests pass. If the garbage collector is called after the second 
iteration the JIT aborts - that's not a crash but it exits; I'll now try to 
find out why.

Original comment by Tobias.N...@gmail.com on 9 Jul 2012 at 3:17

[GoogleCodeExporter]

Here a backtrace when it fails:

Breakpoint 11, 0x001ba5c4 in GC (cx=0x706d30, argc=0, vp=0x1000060) at 
/Volumes/Mac_OS_9/mozilla-aurora/js/src/builtin/TestingFunctions.cpp:26
26  {
(gdb) bt
#0  0x001ba5c4 in GC (cx=0x706d30, argc=0, vp=0x1000060) at 
/Volumes/Mac_OS_9/mozilla-aurora/js/src/builtin/TestingFunctions.cpp:26
#1  0x0025d724 in js::mjit::CallCompiler::generateNativeStub (this=0xefffef38) 
at jscntxtinlines.h:395
#2  0x00253ce0 in js::mjit::ic::NativeCall (f=<value temporarily unavailable, 
due to optimizations>, ic=<value temporarily unavailable, due to 
optimizations>) at 
/Volumes/Mac_OS_9/mozilla-aurora/js/src/methodjit/MonoIC.cpp:1022
#3  0x000179b4 in _JaegerStubVeneer () at 
/Volumes/Mac_OS_9/mozilla-aurora/js/src/methodjit/TrampolinePPCOSX.S:273
#4  0x4e800020 in ?? ()
Cannot access memory at address 0x4e800020
Cannot access memory at address 0x4e800020
Cannot access memory at address 0x8001007c
Previous frame inner to this frame (gdb could not unwind past this frame)

When it doesn't fail the backtrace is OK.

Original comment by Tobias.N...@gmail.com on 9 Jul 2012 at 7:32

[GoogleCodeExporter]

Now it's clear to me why after the call to the garbage collector after the 
first and the second iteration doesn't cause problems: the stubs have only just 
been created but never used until that point.
The garbage collector will only collect stubs that have been executed at least 
once.

The following java script lines cause the problem:
ar[k+5] = { };
ar[k+2] = "3" + k;
The problem seems to be somewhere in the conversion of these values to the 
destination type (which can be any integer or float type).

Original comment by Tobias.N...@gmail.com on 9 Jul 2012 at 11:21

[GoogleCodeExporter]

I suspect the problem is actually in your MacroAssembler methods, specifically 
that you try to use BaseIndex addressing to store tempRegister in your 
store8(Imm...) and store16(Imm...) implementations. Since generateAddressOffset 
may clobber tempRegister, you can't use tempRegister as the input to the 
high-level methods taking a BaseIndex.

Original comment by magef...@gmail.com on 10 Jul 2012 at 12:29

[GoogleCodeExporter]

I have a bigger problem anyway. 15 basically works (modulo issue 166 and some 
Aurora-related crashes which are Mozilla bugs), but dies on exit deep in the 
C++ runtime (__cxa_finalize and friends), suggesting a global object that died 
an early death and then __cxa_finalize tries to call its destructor when it is 
somehow already gone. I could wallpaper this with hacks to nsAppRunner.cpp or 
nsBrowserApp.cpp to install a SIGILL handler or something disgusting, but while 
it's okay for the unstable audience it is not okay for wider release. So I'm 
going to work on that first before I pull anything else into my tree. If I end 
up spinning my wheels, I'll post changesets for perusal.

I assume, Tobias, that your 15 shows no such issue. The question then becomes 
whether it's the SDK or the compiler (or, possibly, my code changes).

Original comment by classi...@floodgap.com on 10 Jul 2012 at 2:39

[GoogleCodeExporter]

Some research suggests (https://bugzilla.redhat.com/show_bug.cgi?id=161061) 
that gcc 4.0 runs into trouble if libstdc++ is ever dlclose()ed -- and more 
generally, library unloading can possibly cause __cxa_finalize crashes. It 
might be worth running the browser with DYLD_PRINT_LIBRARIES_POST_LAUNCH=1 to 
see if there is any strange dlopen()/dlclose() behavior going on....

Original comment by magef...@gmail.com on 10 Jul 2012 at 3:32

[GoogleCodeExporter]

An excellent suggestion, but that was one of the first things I checked (with 
DYLD_PRINT_LIBRARIES in full) and it doesn't look like anything's getting 
unloaded at all, in fact.

The crash is long after main() in nsBrowserApp.cpp has returned, so it's 
clearly a global object that survives XPCOM.

Original comment by classi...@floodgap.com on 10 Jul 2012 at 4:58

[GoogleCodeExporter]

So temporary wallpaper is just to call _Exit() from main() and never let the 
atexit handlers run (or drop into a debugger trap #if DEBUG). That sucks, but 
it works. Messing around with Mach exception handlers just made a big mess.

Original comment by classi...@floodgap.com on 10 Jul 2012 at 5:13

[GoogleCodeExporter]

Farmed off as issue 169. I'll run tests on Ben's patch tomorrow while I'm 
avoiding work.

Original comment by classi...@floodgap.com on 10 Jul 2012 at 5:22

[GoogleCodeExporter]

Rebuilding with the patch from comment 14 results in the same problems here.

Original comment by Tobias.N...@gmail.com on 10 Jul 2012 at 6:31

[GoogleCodeExporter]

Might be another gcc optimization issue: a release build doesn't show the 
problem. I've built my debug builds with -O1.

Original comment by Tobias.N...@gmail.com on 10 Jul 2012 at 7:44

[GoogleCodeExporter]

I guess it could be a compiler thing, since I can't reproduce this. I'm using 
gcc 4.2; I assume, Tobias, that you're using 4.6?

Original comment by magef...@gmail.com on 10 Jul 2012 at 2:54

[GoogleCodeExporter]

Yes, I'm using gcc 4.6 .
Using your patch Aurora 15 completed the full dromaeo test suite without any 
problems.

Original comment by Tobias.N...@gmail.com on 10 Jul 2012 at 3:11

[GoogleCodeExporter]

I just noticed in js/src/configure.in that ARM is still using POLYIC typed 
arrays in 15. What's the powerpc*- options in your configure.in?

Original comment by classi...@floodgap.com on 10 Jul 2012 at 3:18

[GoogleCodeExporter]

issue 165, comment 43:
"While looking for the correct place to do remove the definition I just 
recognized that by copying the JS config flags (in configure.in) from ARM we 
copied something that might be an oversight. ENABLE_POLYIC_TYPED_ARRAY doesn't 
do anything (anymore?) but I think it must be ENABLE_METHODJIT_TYPED_ARRAY 
instead (now?) as all other architectures use that.
In case this really proves to be an oversight and changing the definition 
actually works this should be filed as a mozilla bug."

The ARM can't be "using"  POLYIC typed arrays any more as there isn't any other 
occurrence of it in the sources. Either the ARM port cannot yet use the new way 
of typed arrays or it was simply an oversight.

Original comment by Tobias.N...@gmail.com on 10 Jul 2012 at 3:33

[GoogleCodeExporter]

I think I need to get a better night's sleep before I post lately. Thanks, I 
dimly remember reading that now.

Original comment by classi...@floodgap.com on 10 Jul 2012 at 3:37

[GoogleCodeExporter]

I replaced ENABLE_POLYIC_TYPED_ARRAY with ENABLE_METHODJIT_TYPED_ARRAY in 
configure.in for powerpc*-darwin* .

Original comment by Tobias.N...@gmail.com on 10 Jul 2012 at 3:37

[GoogleCodeExporter]

I built it with Ben's changes, but I get six failures, which are probably all 
related to one bug since the assertion is nearly identical. Notice that type 
inference masks the failure. In your trees, does JaegerStubVeneer still have my 
patch that pulls down an argument area there as well?

bruce:/home/spectre/src/bruce/mozilla-15a/js/src/jit-test/% ./jit_test.py 
--jitflags=am,amn ../../../obj-ff-dbg/dist/bin/js
[4338|   6|   0|4344]    100% =======================================>| 1100.4s
FAILURES:
    -a -m /Volumes/BruceDeuce/src/mozilla-15a/js/src/jit-test/tests/basic/testTypedArrayClamping.js
    -a -m /Volumes/BruceDeuce/src/mozilla-15a/js/src/jit-test/tests/basic/testTypedArrayUint32.js
    -a -m /Volumes/BruceDeuce/src/mozilla-15a/js/src/jit-test/tests/basic/testTypedArrays.js
    -a -m /Volumes/BruceDeuce/src/mozilla-15a/js/src/jit-test/tests/jaeger/bug679666.js
    -a -m /Volumes/BruceDeuce/src/mozilla-15a/js/src/jit-test/tests/jaeger/testSetTypedFloatArray.js
    -a -m /Volumes/BruceDeuce/src/mozilla-15a/js/src/jit-test/tests/jaeger/testSetTypedIntArray.js

Starting program: /Volumes/BruceDeuce/src/mozilla-15a/obj-ff-dbg/dist/bin/js -a 
-m -f lib/prolog.js -f tests/basic/testTypedArrays.js
Reading symbols for shared libraries 
....................+++...............................................+ done
Assertion failure: retval == !isDummyFrame(), at 
/Volumes/BruceDeuce/src/mozilla-15a/js/src/vm/Stack.h:442

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x0039d4a4 in js::StackFrame::script (this=0x132fb2c) at Stack.h:442
442             JS_ASSERT(retval == !isDummyFrame());
(gdb) bt
#0  0x0039d4a4 in js::StackFrame::script (this=0x132fb2c) at Stack.h:442
#1  0x00461040 in js::mjit::EnterMethodJIT (cx=0x1408750, fp=0x132fb2c, 
code=0x1320e70, stackLimit=<value temporarily unavailable, due to 
optimizations>, partial=false) at Stack.h:1282
#2  0x0046297c in js::mjit::JaegerShot (cx=0x1408750, partial=false) at 
/Volumes/BruceDeuce/src/mozilla-15a/js/src/methodjit/MethodJIT.cpp:1079

Starting program: /Volumes/BruceDeuce/src/mozilla-15a/obj-ff-dbg/dist/bin/js -a 
-m -f lib/prolog.js -f tests/basic/testTypedArrayUint32.js
Reading symbols for shared libraries 
....................+++...............................................+ done
Assertion failure: retval == !isDummyFrame(), at 
/Volumes/BruceDeuce/src/mozilla-15a/js/src/vm/Stack.h:442

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x0039d4a4 in js::StackFrame::script (this=0x13237b8) at Stack.h:442
442             JS_ASSERT(retval == !isDummyFrame());

Starting program: /Volumes/BruceDeuce/src/mozilla-15a/obj-ff-dbg/dist/bin/js -a 
-m -f lib/prolog.js -f tests/jaeger/testSetTypedIntArray.js
Reading symbols for shared libraries 
....................+++...............................................+ done
Assertion failure: retval == !isDummyFrame(), at 
/Volumes/BruceDeuce/src/mozilla-15a/js/src/vm/Stack.h:442

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x0039d4a4 in js::StackFrame::script (this=0x13504b0) at Stack.h:442
442             JS_ASSERT(retval == !isDummyFrame());

Original comment by classi...@floodgap.com on 10 Jul 2012 at 5:07

[GoogleCodeExporter]

Type inference disables setelem and getelem typed array stubs; look for 
JS_METHODJIT_TYPED_ARRAY in  PolyIC.cpp .

But wait, does that mean that type inference deprecates 
JS_METHODJIT_TYPED_ARRAY?

Original comment by Tobias.N...@gmail.com on 10 Jul 2012 at 5:33

[GoogleCodeExporter]

Looking at FastOps.cpp shows that in fact type inference deprecates the typed 
array PIC stubs by providing an even faster way to accomplish the same.

Original comment by Tobias.N...@gmail.com on 10 Jul 2012 at 5:45

[GoogleCodeExporter]

Maybe, but we should still try to be at parity with the other backends that 
implement it. Nevertheless, adding a dummy argument area in JaegerStubVeneer 
made no difference and the backtrace implied that wasn't the problem anyhow. 
Are you both linking against the 10.4 or 10.5 SDK?

I'm kind of against the wall here: this week is shot because I need to get 14 
and 10.0.6 built, and the QTE needs testing as part of the browser as well as 
piecing back together WebM. Type inference does seem to supersede the need for 
this and I'm not sure what we're gaining. If I have time to debug this for 15, 
I will, but is there any objection to delaying this for 16?

Original comment by classi...@floodgap.com on 10 Jul 2012 at 5:47

[GoogleCodeExporter]

And that faster paths in FastOps.cpp are guarded by JS_METHODJIT_TYPED_ARRAY as 
well.

Original comment by Tobias.N...@gmail.com on 10 Jul 2012 at 5:48

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Executive decision: backout for 15, reland for 16. I'm keeping the 
BaseAssembler patch though because that makes good sense. We stay with whatever 
we had for the moment.

Original comment by classi...@floodgap.com on 10 Jul 2012 at 6:51

[GoogleCodeExporter]

A build with gcc 4.7 (from macports) causes some fails in the typed array tests 
as well. Later on it couldn't compile some chromium code and the build 
completely failed - so gcc 4.6 seems to be the latest gcc that we can use to 
build.

Original comment by Tobias.N...@gmail.com on 10 Jul 2012 at 7:59