655260 - Burning JaegerMonkey (TI) for ARM.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jacob Bramley [:jbramley]]

There are a few small patches required here, so I've just created one bug to reduce Bugzilla noise. Most of these are probably the result of merges and new development on the type-inference branch.

Comment 1

[Jacob Bramley [:jbramley]]

Attachment: 1: Tweak register name calls on ARM to fix the build.

Comment 2

[Jacob Bramley [:jbramley]]

Attachment: 2: Adjust VMFrame assertions to fix the ARM build.

Comment 3

[Jacob Bramley [:jbramley]]

Attachment: 3: Add absDouble to the ARM back-end. The 'x & -x' trick doesn't look sensible on ARM.

Comment 4

[Jacob Bramley [:jbramley]]

We aren't there yet. There's another failure after adding these three:

/work/moz/mozilla.org/projects/jaegermonkey/js/src/methodjit/FastOps.cpp: In member function 'void js::mjit::Compiler::jsop_stricteq(JSOp)':                                                                                                                                                                                               
/work/moz/mozilla.org/projects/jaegermonkey/js/src/methodjit/FastOps.cpp:1932: error: 'class js::mjit::FrameState' has no member named 'pushSyncedType'

Comment 5

[Paul Biggar]

Comment on attachment 530630 [details] [diff] [review]
1: Tweak register name calls on ARM to fix the build.

Review of attachment 530630 [details] [diff] [review]:
-----------------------------------------------------------------

That's all? Great!

Comment 6

[Paul Biggar]

Comment on attachment 530631 [details] [diff] [review]
2: Adjust VMFrame assertions to fix the ARM build.

Review of attachment 530631 [details] [diff] [review]:
-----------------------------------------------------------------

Don't know this code, gonna leave it to Brian.

Comment 7

[Paul Biggar]

Comment on attachment 530632 [details] [diff] [review]
3: Add absDouble to the ARM back-end. The 'x & -x' trick doesn't look sensible on ARM.

Review of attachment 530632 [details] [diff] [review]:
-----------------------------------------------------------------

Again, not familiar here, so I'm gonna defer to Brian.

Id say abstract the x&-x into an absdouble method on other platforms, but don't know if that's appropriate.

Comment 8

[Jacob Bramley [:jbramley]]

(In reply to comment #7)
> Id say abstract the x&-x into an absdouble method on other platforms, but
> don't know if that's appropriate.

Yep, that would certainly be cleaner!

Comment 9

[Jacob Bramley [:jbramley]]

Attachment: 2: Adjust VMFrame assertions to fix the ARM build.

I had to make a couple of changes to this as a few things got moved around in the repository. It probably doesn't need another full review, but I've got a couple more for review anyway.

Comment 10

[Jacob Bramley [:jbramley]]

Attachment: 3: Add absDouble to the MacroAssembler interface, and abstract the 'x & -x' trick away as it is an x86-specific optimization.

The only change here is that I added absDouble to the x86 back-end, as suggested.

Comment 11

[Jacob Bramley [:jbramley]]

Attachment: 4: Tweak pushSynced call to fix ARM build.

This one really is trivial. It looks like the pushSyncedType method was renamed to overload the pushSynced method. This patch just updates a call site accordingly.

Comment 12

[Jacob Bramley [:jbramley]]

Attachment: 5: Add interpoline support to ARM.

This is the only non-trivial patch required to fix the ARM build. I'm not entirely sure that my interpoline code is correct, but it seems to do everything that the x86 version does.

This is the last patch required to make the type inference tree build on ARM. It still fails several tests, but they'll get separate bugs (and patches).

Comment 13

[Brian Hackett [Laid off!]]

Comment on attachment 531604 [details] [diff] [review]
5: Add interpoline support to ARM.

Review of attachment 531604 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks!

::: js/src/methodjit/MethodJIT.cpp
@@ +609,5 @@
> +SYMBOL_STRING(JaegerInterpolineScripted) ":"        "\n"
> +    /* The only difference between JaegerInterpoline and JaegerInpolineScripted is that the
> +     * scripted variant has to walk up to the previous StackFrame first. */
> +"   ldr     r11, [r11, #(4*4)]"             "\n"    /* Load f->prev_ */
> +"   str     r11, [sp, #(4*7)]"              "\n"    /* Update VMFrame->entryFP */

Comment should be updating f.regs.fp (the offset itself looks right).

@@ +621,5 @@
> +"   mov     r1, r5"                         "\n"    /* returnType */
> +"   mov     r0, r4"                         "\n"    /* returnData */
> +"   blx  " SYMBOL_STRING_RELOC(js_InternalInterpret) "\n"
> +"   cmp     r0, #0"                         "\n"
> +"   ldr     ip, [sp, #(4*7)]"               "\n"    /* Load (StackFrame*)entryFp */

Ditto.

Comment 14

[Jacob Bramley [:jbramley]]

(In reply to comment #13)
> Comment should be updating f.regs.fp (the offset itself looks right).

Ah, yes, I thought that was odd. Thanks! (I'd counted FrameRegs as a single field.)

http://hg.mozilla.org/projects/jaegermonkey/rev/e96dad5f95fd
http://hg.mozilla.org/projects/jaegermonkey/rev/07270a0cbc86
http://hg.mozilla.org/projects/jaegermonkey/rev/5e9fa1b150aa
http://hg.mozilla.org/projects/jaegermonkey/rev/43c304c90149
http://hg.mozilla.org/projects/jaegermonkey/rev/ef221c659ef0