Closed
Bug 507117
Opened 16 years ago
Closed 16 years ago
Merge Tamarin NativeARM changes into Trace Monkey.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
People
(Reporter: jbramley, Assigned: jbramley)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 5 obsolete files)
42.64 KB,
patch
|
graydon
:
review+
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
Attachment #393152 -
Attachment is obsolete: true
Attachment #393507 -
Flags: review?(graydon)
Attachment #393152 -
Flags: review?(graydon)
Assignee | ||
Comment 6•16 years ago
|
||
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•16 years ago
|
Attachment #396194 -
Attachment description: Re-synchronization with tree. → p8v3a: Re-synchronization with tree.
Comment 7•16 years ago
|
||
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 8•16 years ago
|
||
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•16 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 10•16 years ago
|
||
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+
Comment 11•16 years ago
|
||
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
Comment 12•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 13•16 years ago
|
||
status1.9.2:
--- → beta1-fixed
Flags: wanted1.9.2+
Updated•16 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•