506138 - Merge tracemonkey NativeARM changes into tamarin-redux

Status: VERIFIED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P1)

Description

[Rick Reitmaier]

Bring on the goodness

Comment 1

[Rick Reitmaier]

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

Comment 2

[Rick Reitmaier]

Attachment: ver 2 - fixups from v1

This patch is code rearrangement, which eases merging with TM and makes follow-on patches easier to digest.

Comment 3

[Rick Reitmaier]

pushed attachment 390405 [details] [diff] [review] - http://hg.mozilla.org/tamarin-redux/rev/57c424c9efba

Comment 4

[Rick Reitmaier]

Attachment: 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

Comment 5

[Rick Reitmaier]

pushed attachment 390527 [details] [diff] [review] - http://hg.mozilla.org/tamarin-redux/rev/19987199b64c

Comment 6

[Rick Reitmaier]

Attachment: p3v1 - introduce asm_reg

introduce asm_arg and consolidate the the EABI code.

Comment 7

[Rick Reitmaier]

pushed attachment 390597 [details] [diff] [review] - http://hg.mozilla.org/tamarin-redux/rev/869fb267c724

Comment 8

[Rick Reitmaier]

Attachment: 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

Comment 9

[Rick Reitmaier]

pushed attachment 390605 [details] [diff] [review] - http://hg.mozilla.org/tamarin-redux/rev/678b28494b49

Comment 10

[Rick Reitmaier]

Attachment: p5v1 - instruction changes from header

Comment 11

[Rick Reitmaier]

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

Comment 12

[Rick Reitmaier]

Attachment: p6v1 - ld/sub/add_imm changes

Comment 13

[Rick Reitmaier]

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

no + review ; pre-approved from edwin.

Comment 14

[Jacob Bramley [:jbramley]]

+// 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.

Comment 15

[Rick Reitmaier]

Attachment: p7v1

more cleanup

Comment 16

[Rick Reitmaier]

pushed attachment #391249 [details] [diff] [review] - http://hg.mozilla.org/tamarin-redux/rev/78897b978d4c

Comment 17

[Jacob Bramley [:jbramley]]

Attachment: 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.

Comment 18

[Rick Reitmaier]

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.

Comment 19

[Jacob Bramley [:jbramley]]

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

Comment 20

[Jacob Bramley [:jbramley]]

Attachment: 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 21

[Edwin Smith]

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

Comment 22

[Rick Reitmaier]

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.

Comment 23

[Jacob Bramley [:jbramley]]

(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

[Edwin Smith]

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

Comment 25

[Rick Reitmaier]

Attachment: 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

[Edwin Smith]

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

[Steven Johnson]

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

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

Comment 28

[Rick Reitmaier]

Attachment: 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.

Comment 29

[Graydon Hoare :graydon]

Oop, I'm just finishing up for the evening; handing to njn if he wants it.

Comment 30

[Nicholas Nethercote [inactive]]

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.

Comment 31

[Rick Reitmaier]

Attachment: arg stack - patch 1

Comment 32

[Rick Reitmaier]

Attachment: call - patch 2

Comment 33

[Rick Reitmaier]

Attachment: quad fixup - patch 3

Comment 34

[Rick Reitmaier]

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

Comment 35

[Edwin Smith]

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

Comment 36

[Rick Reitmaier]

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

Comment 37

[Rick Reitmaier]

Attachment: asm_load64

Comment 38

[Rick Reitmaier]

Attachment: asm_store64

Comment 39

[Rick Reitmaier]

Attachment: BranchWithLink

Comment 40

[Rick Reitmaier]

Attachment: genPrologue

Comment 41

[Rick Reitmaier]

Attachment: genEpilogue

Comment 42

[Rick Reitmaier]

Attachment: nFragExit

marking p8v3 obsolete as the following patches contain the remaining deltas

Comment 43

[Rick Reitmaier]

Attachment: genPrologue

previous genPrologue patch was incorrect.

Comment 44

[Rick Reitmaier]

pushed changes labeled patch 1/2/3 from above:
http://hg.mozilla.org/tamarin-redux/rev/ec94a162bea0
http://hg.mozilla.org/tamarin-redux/rev/5ce927acdb12
http://hg.mozilla.org/tamarin-redux/rev/80ee62cac3ce

Comment 45

[Rick Reitmaier]

pushed remaining changes : 
   http://hg.mozilla.org/tamarin-redux/rev/72c32b560c28
to  
   http://hg.mozilla.org/tamarin-redux/rev/566281bae8cc

Comment 46

[Edwin Smith]

marking this fixed since the merge is done as of last night.

Comment 47

[Rick Reitmaier]

removing bug 520905 as dependency ... bug 526914 now tracks it.