Merge tracemonkey NativeARM changes into tamarin-redux

VERIFIED FIXED in flash10.1

Status

P1
normal
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: rreitmai, Assigned: rreitmai)

Tracking

unspecified
flash10.1
ARM
All
Dependency tree / graph

Details

Attachments

(17 attachments, 7 obsolete attachments)

59.07 KB, patch
edwsmith
: superreview+
Details | Diff | Splinter Review
25.29 KB, patch
edwsmith
: superreview+
Details | Diff | Splinter Review
9.82 KB, patch
edwsmith
: superreview+
Details | Diff | Splinter Review
18.24 KB, patch
edwsmith
: superreview+
Details | Diff | Splinter Review
18.06 KB, patch
Details | Diff | Splinter Review
36.52 KB, patch
Details | Diff | Splinter Review
4.18 KB, patch
Details | Diff | Splinter Review
4.32 KB, patch
njn
: review+
Details | Diff | Splinter Review
24.41 KB, patch
edwsmith
: review+
Details | Diff | Splinter Review
949 bytes, patch
edwsmith
: review+
Details | Diff | Splinter Review
5.83 KB, patch
edwsmith
: superreview+
Details | Diff | Splinter Review
2.07 KB, patch
Details | Diff | Splinter Review
2.60 KB, patch
Details | Diff | Splinter Review
12.66 KB, patch
Details | Diff | Splinter Review
1.09 KB, patch
Details | Diff | Splinter Review
646 bytes, patch
Details | Diff | Splinter Review
2.31 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
Bring on the goodness
(Assignee)

Updated

9 years ago
Blocks: 503556
(Assignee)

Comment 1

9 years ago
Created attachment 390394 [details] [diff] [review]
ver 1 - code movement

This first patch simply moves code around so it better lines up with the tracemonkey source.

There should be no other differences (except maybe for a function or two added).
(Assignee)

Comment 2

9 years ago
Created attachment 390405 [details] [diff] [review]
ver 2 - fixups from v1


This patch is code rearrangement, which eases merging with TM and makes follow-on patches easier to digest.
Attachment #390394 - Attachment is obsolete: true
Attachment #390405 - Flags: superreview?(edwsmith)

Updated

9 years ago
Attachment #390405 - Flags: superreview?(edwsmith) → superreview+
(Assignee)

Comment 4

9 years ago
Created attachment 390527 [details] [diff] [review]
p2v1 - immediates

Brings over the following from tracemonkey:
(1) CountLeadingZero's used in regalloc and asm_mmq
(2) asm_mmq code 
(3) nFragExit asm_loop directory copied over
(4) imm64_x usage
(5) asm_ld / asm_param / asm_int optimizations
Attachment #390527 - Flags: superreview?(edwsmith)
(Assignee)

Comment 6

9 years ago
Created attachment 390597 [details] [diff] [review]
p3v1 - introduce asm_reg

introduce asm_arg and consolidate the the EABI code.
Attachment #390597 - Flags: superreview?
(Assignee)

Updated

9 years ago
Attachment #390597 - Attachment is patch: true
Attachment #390597 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

9 years ago
Attachment #390597 - Flags: superreview? → superreview?(edwsmith)
(Assignee)

Comment 8

9 years ago
Created attachment 390605 [details] [diff] [review]
p4v1 - asm_branch and other adjustments

Includes: 

(1) shuffle EABI code from asm_call to asm_arg
(2) imm64_x usage
(3) LD32_nochk / asm_ld_chk optimizations
(4) asm_branch comments from TM added
Attachment #390605 - Flags: superreview?(edwsmith)
(Assignee)

Comment 10

9 years ago
Created attachment 390982 [details] [diff] [review]
p5v1 - instruction changes from header
(Assignee)

Comment 11

9 years ago
Pushed p5v1 (attachment #390982 [details] [diff] [review]) - no + review, pre-approved from edwin.
(Assignee)

Comment 12

9 years ago
Created attachment 390983 [details] [diff] [review]
p6v1 - ld/sub/add_imm changes
(Assignee)

Comment 13

9 years ago
Pushed p6v1 (attachment #390983 [details] [diff] [review]) - http://hg.mozilla.org/tamarin-redux/rev/4179457f88d9.

no + review ; pre-approved from edwin.
+// NB the thumb2 check (i.e using > v6) is technically 
+// not correct, but its close enough for now).

In Trace Monkey, AvmCore.thumb2 is set based on some tests. For Linux, the test uses auxv, and there is no way to detect ARMv6T2 that I know of. Thus, we ignore ARMv6T2 and go with ARMv7+. This is reasonable as ARMv6T2 cores are rare in practice.

For WinCE, however, the detection mechanism uses undefined instruction aborts to detect features, so it will catch ARMv6T2 correctly. We may be able to do something similar for Linux, but I didn't think it worth investigating in detail, given the rarity of ARMv6T2 in the kind of device we're targetting.
(Assignee)

Comment 15

9 years ago
Created attachment 391249 [details] [diff] [review]
p7v1

more cleanup
Created attachment 391626 [details] [diff] [review]
p8v1: Merge asm_call and asm_arg.

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

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 507117, which makes similar changes to Trace Monkey.

----

I'm not entirely sure how to test Tamarin as the stuff in the "test" directory doesn't seem to work for me. I've checked the patch using a simple "hello world"-type program and -Djitordie, and it seems to be Ok. In addition, it works in Trace Monkey with trace-test.js, though there are bits in Tamarin that this won't catch.

Rick: I worked from you work-in-progress repository, but this also applies and works on tamarin-redux.

----

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

Comment 18

9 years ago
Comment on attachment 391626 [details] [diff] [review]
p8v1: Merge asm_call and asm_arg.

'-'ing only because there are a few issues with the patch, but overall it is how we want the code to evolve.

- builds* on runs on beagle-board (gcc/debian 4.3.2) with --enable-arm-fpu but fails a few of the acceptance tests:
 ecma3/GlobalObject/e15_1_2_2_1
 ecma3/String/e15_5_4_5_4
 ecma3/String/e15_5_3_2_1
 ecma3/Math/e15_8_2_11

- fails to build on winmo 

* after adding a missing IS_ARM_ARCH_VFP() 

I'm investigating the build failure on winmo and the test failures.  

Will post a follow-up with findings.
Attachment #391626 - Flags: review?(rreitmai) → review-
Created attachment 392960 [details] [diff] [review]
p8v2: Merge asm_call. Addresses Rick's feedback.

This addresses the issues you mentioned and fixes the regressions.

I've run the test suite without --enable-arm-vfp, but I haven't yet run it with that flag as the test suite takes a long time to run on my ARM platform. I will run it overnight and report back if there were any problems, but the code is now pretty much identical to that in Trace Monkey and it works there. (The regressions were not in the VFP code.)

I don't know if it fixes the Windows Mobile build. I've moved some code around, so it may now work. If it doesn't could you post the build error please? (I don't have a WinCE build environment.)
Attachment #391626 - Attachment is obsolete: true
Attachment #392960 - Flags: review?(edwsmith)
Created attachment 392966 [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 #392960 - Attachment is obsolete: true
Attachment #392966 - Flags: review?(edwsmith)
Attachment #392960 - Flags: review?(edwsmith)

Comment 21

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

Nothing obviously wrong jumps out, so marking this + on the evidence that it's matching TM and passing both test suites.

I will test this on WinMo and if things look good, push it into tamarin-redux.  (we're about to cut a release to tamarin-central so this will go in after that release).
Attachment #392966 - Flags: review?(edwsmith) → review+

Updated

9 years ago
Attachment #390527 - Flags: superreview?(edwsmith) → superreview+

Updated

9 years ago
Attachment #390597 - Flags: superreview?(edwsmith) → superreview+

Updated

9 years ago
Attachment #390605 - Flags: superreview?(edwsmith) → superreview+
(Assignee)

Comment 22

9 years ago
comments on attachment 392966 [details] [diff] [review]:

(1) asm_arg_64() doesn't preserve NJ_SOFTFLOAT.  Do we want to remove this as a compilation option?  While the logic may be correct (I haven't reviewed it in detail yet) I believe it will add to the size.

(2) asm_stk_arg() similarly removes NJ_ARM_VFP .  I wonder if we could simply change if (!isQuad) to (!isQuad || !IS_ARM_ARCH_VFP())  or somesuch.

(3) V4T and V5 support existed in the old branch logic.  It looks like it has been removed.
(In reply to comment #22)
> comments on attachment 392966 [details] [diff] [review]:
> 
> (1) asm_arg_64() doesn't preserve NJ_SOFTFLOAT.  Do we want to remove this as a
> compilation option?  While the logic may be correct (I haven't reviewed it in
> detail yet) I believe it will add to the size.

Indeed. This comes back to the discussion or whether to use runtime-detection (as in Trace Monkey) or build-time-detection (as in Tamarin). Personally, I prefer the former; one of the primary benefits of a JIT compiler is that it can generate code that suits the target platform, regardless of the compile-time options. We can, for example, build an ARMv5 non-VFP VM that we can distribute to everyone, and still have the JIT compiler take advantage of VFP at run-time.

Yes, the price for this is that the code size increases, but it isn't really a significant increase. In addition, _most_ (but not all) target platforms will have VFP support, so in the majority of cases the code will actually be used.

> (2) asm_stk_arg() similarly removes NJ_ARM_VFP .  I wonder if we could simply
> change if (!isQuad) to (!isQuad || !IS_ARM_ARCH_VFP())  or somesuch.

Again, the proper way to proceed here depends heavily on the decisions taken for the configuration mechanism.

> (3) V4T and V5 support existed in the old branch logic.  It looks like it has
> been removed.

It still supports ARMv5, but not ARMv4 or ARMv4T. We removed this from Trace Monkey some time ago, and I think we agreed to remove it from Tamarin too. (I don't think I would have intentionally removed it without discussing it first, but it was a while back now so I can't be sure without looking at IRC logs and the like.)

If you want it to remain, that's fine, but we'll need to find a way of testing it, and I'd argue that the value of ARMv4(T) support is limited as no-one is going to want to run Trace Monkey on such devices, let alone Firefox.

Comment 24

9 years ago
ARMv5 as a minimum is okay for Tamarin

with respect to cpu detection (ARMv5/6/7) it seems fine to do runtime detection, and we can add additional code to optimize it out when we want both a) compile-time decision, and b) to reduce code footprint.

regarding softfloat, runtime detection seems fine, to get us merged.

we will certianly want to have some kind of ability to compile in the decision to use VFP however, because when VFP is available, the hosts we embed in will want to take advantage of it in their C/C++ code.  and if it's compiled into C/C++ i see no reason for it not to be compiled into the jit, too.  the same is true for interworking -- if its something you must enable/disable in the C/C++ compiler then having it be a runtime decision in the jit doesn't seem to have a scenario where it pays off.  (maybe interworking is already done this way.. not checking, just making the point).
(Assignee)

Comment 25

9 years ago
Created attachment 403842 [details] [diff] [review]
asm_arg/stk changes 

This is a partial merge/recoding of some of the changes residing in TM.   It pulls apart asm_arg into asm_arg_64 and removes many ifdefs for vfp, replacing them with IS_ARM_ARCH_VFP() which for tamarin is a constant but for tracemonkey its a dynamic access.

Comment 26

9 years ago
can we have a declaration somewhere in NativeARM.h that is "const" in TM and not in TR, but just plainly access the variable in code?  cleans things up and should produce the same code if the C++ compiler is any good.

Comment 27

9 years ago
(In reply to comment #26)
> if the C++ compiler is any good.

Ha ha ha! Edwin, you're so funny :-)
(Assignee)

Comment 28

9 years ago
Created attachment 403922 [details] [diff] [review]
applies to TM 


This patch applies to nanojit-central and cleans up the macros in NativeARM.h so that gcc no longer complains about them being empty in their 'else' statements.

Please land on the tracemonkey side assuming no ill effects.
Attachment #403922 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #403922 - Flags: review? → review?(graydon)
Oop, I'm just finishing up for the evening; handing to njn if he wants it.

Updated

9 years ago
Attachment #403922 - Flags: review?(graydon) → review?(nnethercote)
Comment on attachment 403922 [details] [diff] [review]
applies to TM 

I've filed bug 519876 for the TM patch so it can be tracked separately.  Thanks.
Attachment #403922 - Flags: review?(nnethercote) → review+
(Assignee)

Comment 31

9 years ago
Created attachment 404194 [details] [diff] [review]
arg stack - patch 1
Attachment #403842 - Attachment is obsolete: true
(Assignee)

Comment 32

9 years ago
Created attachment 404195 [details] [diff] [review]
call - patch 2
(Assignee)

Comment 33

9 years ago
Created attachment 404196 [details] [diff] [review]
quad fixup - patch 3
(Assignee)

Updated

9 years ago
Attachment #404194 - Flags: review?(edwsmith)
(Assignee)

Updated

9 years ago
Attachment #404196 - Flags: review?(edwsmith)
(Assignee)

Updated

9 years ago
Depends on: 520688
(Assignee)

Comment 34

9 years ago
Created attachment 404730 [details] [diff] [review]
asm_call - patch 2 

keeps the existing logic rather than replacing with TM logic which fails regressions.  Need to circle back to this issue (bug 520688).
Attachment #404195 - Attachment is obsolete: true
Attachment #404730 - Flags: superreview?(edwsmith)

Comment 35

9 years ago
Comment on attachment 404194 [details] [diff] [review]
arg stack - patch 1

asm_fcmp shouldn't even be in Assembler.h, its specific to each backend.

NativeARM.cpp:204 whitespace deleted, presumably to converge, but it messes up indentation.

line 514: check for LC_Assembly removed.  not sure why, just convergence with TM?

line 549, the comment would be nice to keep unless its just a blatant lie, in which case we should fix it instead of deleting it

line 567: comment should be NJ_MERGE instead of TM_MERGE
Attachment #404194 - Flags: review?(edwsmith) → review+

Updated

9 years ago
Attachment #404196 - Flags: review?(edwsmith) → review+

Updated

9 years ago
Attachment #404730 - Flags: superreview?(edwsmith) → superreview+
(Assignee)

Updated

9 years ago
Depends on: 520905
(Assignee)

Comment 36

9 years ago
re: 
 asm_fcmp() - ok, i'll remove it in an upcoming patch
 whitespace - yes, the whitespace issue is temporary
 LC_Assembly - yes matches TM
 comment - ok, i'll add it back before landing
 NJ_MERGE - ok its just a temporary placeholder anyways
(Assignee)

Comment 37

9 years ago
Created attachment 405067 [details] [diff] [review]
asm_load64
Attachment #405067 - Flags: superreview?(edwsmith)
(Assignee)

Comment 38

9 years ago
Created attachment 405068 [details] [diff] [review]
asm_store64
Attachment #405068 - Flags: superreview?(edwsmith)
(Assignee)

Comment 39

9 years ago
Created attachment 405069 [details] [diff] [review]
BranchWithLink
Attachment #405069 - Flags: superreview?(edwsmith)
(Assignee)

Comment 40

9 years ago
Created attachment 405071 [details] [diff] [review]
genPrologue
Attachment #405071 - Flags: superreview?(edwsmith)
(Assignee)

Comment 41

9 years ago
Created attachment 405072 [details] [diff] [review]
genEpilogue
Attachment #405072 - Flags: superreview?(edwsmith)
(Assignee)

Comment 42

9 years ago
Created attachment 405073 [details] [diff] [review]
nFragExit

marking p8v3 obsolete as the following patches contain the remaining deltas
Attachment #392966 - Attachment is obsolete: true
Attachment #405073 - Flags: superreview?(edwsmith)
(Assignee)

Comment 43

9 years ago
Created attachment 405074 [details] [diff] [review]
genPrologue

previous genPrologue patch was incorrect.
Attachment #405071 - Attachment is obsolete: true
Attachment #405074 - Flags: superreview?(edwsmith)
Attachment #405071 - Flags: superreview?(edwsmith)
(Assignee)

Updated

9 years ago
Depends on: 522341

Updated

9 years ago
Depends on: 522377
(Assignee)

Updated

9 years ago
Attachment #405067 - Flags: superreview?(edwsmith)
(Assignee)

Updated

9 years ago
Attachment #405068 - Flags: superreview?(edwsmith)
(Assignee)

Updated

9 years ago
Attachment #405069 - Flags: superreview?(edwsmith)
(Assignee)

Updated

9 years ago
Attachment #405072 - Flags: superreview?(edwsmith)
(Assignee)

Updated

9 years ago
Attachment #405073 - Flags: superreview?(edwsmith)
(Assignee)

Updated

9 years ago
Attachment #405074 - Flags: superreview?(edwsmith)

Updated

9 years ago
Priority: -- → P1
Target Milestone: --- → flash10.1

Comment 46

9 years ago
marking this fixed since the merge is done as of last night.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 47

9 years ago
removing bug 520905 as dependency ... bug 526914 now tracks it.
No longer depends on: 520905

Updated

9 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.