Burning JaegerMonkey (TI) for ARM.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
blocker
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jbramley, Assigned: jbramley)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 530630 [details] [diff] [review]
1: Tweak register name calls on ARM to fix the build.
Attachment #530630 - Flags: review?(pbiggar)
(Assignee)

Comment 2

6 years ago
Created attachment 530631 [details] [diff] [review]
2: Adjust VMFrame assertions to fix the ARM build.
Attachment #530631 - Flags: review?(pbiggar)
(Assignee)

Comment 3

6 years ago
Created attachment 530632 [details] [diff] [review]
3: Add absDouble to the ARM back-end. The 'x & -x' trick doesn't look sensible on ARM.
Attachment #530632 - Flags: review?(pbiggar)
(Assignee)

Comment 4

6 years ago
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

6 years ago
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!
Attachment #530630 - Flags: review?(pbiggar) → review+

Comment 6

6 years ago
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.
Attachment #530631 - Flags: review?(pbiggar) → review?(bhackett1024)

Comment 7

6 years ago
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.
Attachment #530632 - Flags: review?(pbiggar) → review?(bhackett1024)
(Assignee)

Comment 8

6 years ago
(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!
Attachment #530631 - Flags: review?(bhackett1024) → review+
Attachment #530632 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 9

6 years ago
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.
Attachment #530631 - Attachment is obsolete: true
Attachment #531599 - Flags: review?(bhackett1024)
(Assignee)

Comment 10

6 years ago
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.
Attachment #530632 - Attachment is obsolete: true
Attachment #531601 - Flags: review?(bhackett1024)
(Assignee)

Comment 11

6 years ago
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.
Attachment #531603 - Flags: review?(bhackett1024)
(Assignee)

Comment 12

6 years ago
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).
Attachment #531604 - Flags: review?(bhackett1024)
Attachment #531599 - Flags: review?(bhackett1024) → review+
Attachment #531601 - Flags: review?(bhackett1024) → review+
Attachment #531603 - Flags: review?(bhackett1024) → review+
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.
Attachment #531604 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 14

6 years ago
(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
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.