Merge Tamarin NativeARM changes into Trace Monkey.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jbramley, Assigned: jbramley)

Tracking

unspecified
ARM
All
Points:
---
Bug Flags:
wanted1.9.2 +
in-testsuite -

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

9 years ago
This bug is to Trace Monkey what bug 506138 is to Tamarin.

It's inevitable that some changes will need to be made to Trace Monkey based on the merging of the NativeARM back-end, so I have created this bug to track those changes separately from the changes required for Tamarin.
(Assignee)

Comment 1

9 years ago
Created attachment 391625 [details] [diff] [review]
p8v1: Merge asm_call and asm_arg.

This merges Tamarin's asm_call and asm_arg with that in Trace Monkey.

There is still some divergence where Tamarin uses static feature detection and Trace Monkey uses run-time feature detection, but that will be the subject of a future patch.

In addition, there are several TODOs in the code as there are a few outstanding questions about which features we want to merge and that kind of thing.

This patch is linked with p8v1 in bug 506138, which makes similar changes to Tamarin.

----

I can't test this on WinCE, so I don't know if the legacy ABI code works.
Attachment #391625 - Flags: review?(graydon)
(Assignee)

Comment 2

9 years ago
Created attachment 392961 [details] [diff] [review]
p8v2: An updated patch to match a bug-fix in the corresponding Tamarin patch.

The original patch had a missing underrunProtect, causing a failure in the Tamarin test suite. This never actually affected Trace Monkey as the bug only manifests on indirect branches.

I haven't (and can't) test the WinCE (non-EABI) code, as I don't have a build environment or platform for it.
Attachment #391625 - Attachment is obsolete: true
Attachment #392961 - Flags: review?(graydon)
Attachment #391625 - Flags: review?(graydon)
(Assignee)

Comment 3

9 years ago
Created attachment 392965 [details] [diff] [review]
p8v3: Fixes alignment of 64-bit EABI arguments.

Sorry, I missed a bit. My updated patch did not check the alignment for 64-bit arguments on the stack (for EABI).

This isn't hit by any test case, but it would almost certinaly cause a crash if ever encountered.
Attachment #392961 - Attachment is obsolete: true
Attachment #392965 - Flags: review?(graydon)
Attachment #392961 - Flags: review?(graydon)
(Assignee)

Comment 4

9 years ago
Created attachment 393152 [details] [diff] [review]
p8v3: Correct patch.

I'll get it right eventually. Apologies for the Bugzilla noise.

The previous one clobbered some stuff in trace-test, for some reason. I think Mercurial got confused, or I got confused with Mercurial. (The latter is more likely.)
Attachment #392965 - Attachment is obsolete: true
Attachment #393152 - Flags: review?(graydon)
Attachment #392965 - Flags: review?(graydon)
(Assignee)

Comment 5

9 years ago
Created attachment 393507 [details] [diff] [review]
p8v3: Re-synchronize with Trace Monkey's tip.
Attachment #393152 - Attachment is obsolete: true
Attachment #393507 - Flags: review?(graydon)
Attachment #393152 - Flags: review?(graydon)
(Assignee)

Comment 6

9 years ago
Created attachment 396194 [details] [diff] [review]
p8v3a: Re-synchronization with tree.

Re-synchronized with the tree. I re-tested only with VFP enabled on a Cortex-A8 platform; there were no regressions.

This is, as far as I'm aware, completely untested on WinCE.
Attachment #393507 - Attachment is obsolete: true
Attachment #396194 - Flags: review?(graydon)
Attachment #393507 - Flags: review?(graydon)
(Assignee)

Updated

9 years ago
Attachment #396194 - Attachment description: Re-synchronization with tree. → p8v3a: Re-synchronization with tree.
Comment on attachment 396194 [details] [diff] [review]
p8v3a: Re-synchronization with tree.

Looks pretty good to me; I'm giving it a test on an ARM emulator here that might have a different configuration just for extra coverage but I assume you know what you're doing.

At least a couple of the TODOs should get cleaned up in the near future (isIndirect() and icall) but I don't mind landing this first and fixing those after.

Do you have commit rights, or do you want me to commit this for you?
Attachment #396194 - Flags: review?(graydon) → review+
Comment on attachment 396194 [details] [diff] [review]
p8v3a: Re-synchronization with tree.

Actually I'm a little confused here, why are you moving the declarations of asm_cmp and asm_fcmp?
Attachment #396194 - Flags: review+ → review?(graydon)
(Assignee)

Comment 9

9 years ago
(In reply to comment #8)
> (From update of attachment 396194 [details] [diff] [review])
> Actually I'm a little confused here, why are you moving the declarations of
> asm_cmp and asm_fcmp?

Tamarin had* asm_cmp in Native<ARCH>.h and asm_fcmp in Assembler.h. The latter was also within an #ifndef NJ_SOFTFLOAT block, which doesn't work in Trace Monkey as we do dynamic detection. I had to move it somewhere (to allow me to reduce divergence), so I moved it next to asm_cmp. Indeed, this makes more sense anyway as neither function is called from anywhere outside their respective Native<ARCH>.cpp files.

* Note that my patch (in bug 506138) hasn't yet been pushed.

The change to Trace Monkey simply matches the new Tamarin implementation.

Does that make sense? If there is something wrong here, let me know and I'll change it (and the corresponding Tamarin patch).

> At least a couple of the TODOs should get cleaned up in the near future
> (isIndirect() and icall) but I don't mind landing this first and fixing those
> after.

Indeed. These irritate me as I don't like pushing what appears to be incomplete (if functional) code, but I want to reduce the divergence as quickly as possible as the TODOs can be resolved once we have nanojit-central up and running.

> Do you have commit rights, or do you want me to commit this for you?

I can push to tracemonkey.
Comment on attachment 396194 [details] [diff] [review]
p8v3a: Re-synchronization with tree.

Ah, divergence-reduction only. Good enough, thanks. Fire when ready.
Attachment #396194 - Flags: review?(graydon) → review+
Actually I had the time this afternoon so I landed it. Hope you don't mind (still has your name on it :)

http://hg.mozilla.org/tracemonkey/rev/0e34f399c64c
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/0e34f399c64c
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 13

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e920c8202ec6
status1.9.2: --- → beta1-fixed
Flags: wanted1.9.2+

Updated

9 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.