Last Comment Bug 655260 - Burning JaegerMonkey (TI) for ARM.
: Burning JaegerMonkey (TI) for ARM.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Android
: -- blocker (vote)
: ---
Assigned To: Jacob Bramley [:jbramley]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-06 08:08 PDT by Jacob Bramley [:jbramley]
Modified: 2011-05-19 08:21 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
1: Tweak register name calls on ARM to fix the build. (2.07 KB, patch)
2011-05-06 08:11 PDT, Jacob Bramley [:jbramley]
paul.biggar: review+
Details | Diff | Splinter Review
2: Adjust VMFrame assertions to fix the ARM build. (3.21 KB, patch)
2011-05-06 08:11 PDT, Jacob Bramley [:jbramley]
bhackett1024: review+
Details | Diff | Splinter Review
3: Add absDouble to the ARM back-end. The 'x & -x' trick doesn't look sensible on ARM. (3.46 KB, patch)
2011-05-06 08:12 PDT, Jacob Bramley [:jbramley]
bhackett1024: review+
Details | Diff | Splinter Review
2: Adjust VMFrame assertions to fix the ARM build. (8.20 KB, patch)
2011-05-11 06:00 PDT, Jacob Bramley [:jbramley]
bhackett1024: review+
Details | Diff | Splinter Review
3: Add absDouble to the MacroAssembler interface, and abstract the 'x & -x' trick away as it is an x86-specific optimization. (4.33 KB, patch)
2011-05-11 06:03 PDT, Jacob Bramley [:jbramley]
bhackett1024: review+
Details | Diff | Splinter Review
4: Tweak pushSynced call to fix ARM build. (646 bytes, patch)
2011-05-11 06:06 PDT, Jacob Bramley [:jbramley]
bhackett1024: review+
Details | Diff | Splinter Review
5: Add interpoline support to ARM. (4.71 KB, patch)
2011-05-11 06:14 PDT, Jacob Bramley [:jbramley]
bhackett1024: review+
Details | Diff | Splinter Review

Description Jacob Bramley [:jbramley] 2011-05-06 08:08:09 PDT
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] 2011-05-06 08:11:07 PDT
Created attachment 530630 [details] [diff] [review]
1: Tweak register name calls on ARM to fix the build.
Comment 2 Jacob Bramley [:jbramley] 2011-05-06 08:11:44 PDT
Created attachment 530631 [details] [diff] [review]
2: Adjust VMFrame assertions to fix the ARM build.
Comment 3 Jacob Bramley [:jbramley] 2011-05-06 08:12:20 PDT
Created attachment 530632 [details] [diff] [review]
3: Add absDouble to the ARM back-end. The 'x & -x' trick doesn't look sensible on ARM.
Comment 4 Jacob Bramley [:jbramley] 2011-05-06 08:13:02 PDT
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 2011-05-06 08:22:10 PDT
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 2011-05-06 08:32:29 PDT
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 2011-05-06 08:35:17 PDT
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] 2011-05-06 08:56:23 PDT
(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] 2011-05-11 06:00:03 PDT
Created attachment 531599 [details] [diff] [review]
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] 2011-05-11 06:03:33 PDT
Created attachment 531601 [details] [diff] [review]
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] 2011-05-11 06:06:32 PDT
Created attachment 531603 [details] [diff] [review]
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] 2011-05-11 06:14:26 PDT
Created attachment 531604 [details] [diff] [review]
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 (:bhackett) 2011-05-11 06:52:31 PDT
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] 2011-05-11 07:22:49 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.