507117 - Merge Tamarin NativeARM changes into Trace Monkey.

Status: RESOLVED FIXED

Description

[Jacob Bramley [:jbramley]]

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.

Comment 1

[Jacob Bramley [:jbramley]]

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.

Comment 2

[Jacob Bramley [:jbramley]]

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.

Comment 3

[Jacob Bramley [:jbramley]]

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.

Comment 4

[Jacob Bramley [:jbramley]]

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.)

Comment 5

[Jacob Bramley [:jbramley]]

Created attachment 393507 [details] [diff] [review]
p8v3: Re-synchronize with Trace Monkey's tip.

Comment 6

[Jacob Bramley [:jbramley]]

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.

Comment 7

[Graydon Hoare :graydon]

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?

Comment 8

[Graydon Hoare :graydon]

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?

Comment 9

[Jacob Bramley [:jbramley]]

(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 10

[Graydon Hoare :graydon]

Comment on attachment 396194 [details] [diff] [review]
p8v3a: Re-synchronization with tree.

Ah, divergence-reduction only. Good enough, thanks. Fire when ready.

Comment 11

[Graydon Hoare :graydon]

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

Comment 12

[Blake Kaplan (:mrbkap)]

http://hg.mozilla.org/mozilla-central/rev/0e34f399c64c

Comment 13

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e920c8202ec6