483962 - Remove ARM-specific code from jstracer.cpp.

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jacob Bramley [:jbramley]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9.0.7) Gecko/2009030422 Ubuntu/8.04 (hardy) Firefox/3.0.7
Build Identifier: http://hg.mozilla.org/tracemonkey/file/809a092500db

The file "js/src/jstracer.cpp" appears to be mostly generally architecture-independent, but some patch-up code exists to make shift operations work as expected on ARM*. In Tamarin, this was fixed by inserting an extra native instruction in NativeARM.cpp, and this method is more consistent with the way that Trace Monkey works.

This patch _may_ improve performance a tiny amount as it removes an LIR instruction for every shift operation that is performed, but in practice it won't have any noticeable effect. This is a tidy-up bug.

* JavaScript's shift instruction uses only the bottom 5 bits of the shift operand, and x86 does the same (I believe). On ARM, however, the whole bottom byte of the shift operand is used, and so an extra AND instruction is required to match the specified JavaScript behaviour.

Reproducible: Always

Comment 1

[Jacob Bramley [:jbramley]]

Attachment: Patch to move the extra AND instruction to NativeARM.cpp.

Patch added. Also adds ANDri to NativeARM.h to allow rX = rY & imm. Passes all tests that passed previously.

Comment 2

[Jacob Bramley [:jbramley]]

Attachment: Repaired patch.

Sorry, I did the diff backwards. New patch attached.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 367990 [details] [diff] [review]
Repaired patch.

I think that this part is right, but I think you also need to modify the SHLi/SARi/SHRi macros to add the &0x1f to the immediate value.. otherwise if the rhs value is a constant, we'll get a bogus instruction being generated since they don't mask.

But I think some more stuff will need to be changed -- the start of asm_arith sets forceReg to true if rhs->constval() is not a U8.. but that doesn't need to be true for shifts, since we'll mask it anyway.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oh, sounds like we need to add another test to trace-tests to catch this, if nothing broke... I think a shift with a value > 31 but less than 255 should cause brokenness.

Comment 5

[Jacob Bramley [:jbramley]]

(In reply to comment #3)
> (From update of attachment 367990 [details] [diff] [review])
> I think that this part is right, but I think you also need to modify the
> SHLi/SARi/SHRi macros to add the &0x1f to the immediate value.. otherwise if
> the rhs value is a constant, we'll get a bogus instruction being generated
> since they don't mask.

Good point. I added them in NativeARM.cpp but they'd be much better in the macros themselves.

I will produce an updated patch shortly.

> But I think some more stuff will need to be changed -- the start of asm_arith
> sets forceReg to true if rhs->constval() is not a U8.. but that doesn't need to
> be true for shifts, since we'll mask it anyway.

Ah yes. I hadn't spotted that. This is a simple fix so I'll add it to the patch.

Comment 6

[Jacob Bramley [:jbramley]]

Attachment: Move extra AND instruction to NativeARM.cpp and add some trace tests to test it.

* Move the ARM-specific extra AND instruction to NativeARM.cpp.
* Never force register assignment for shift instructions as they're all masked to 0x1f anyway, and will always fit in an ARM immediate value.
* Modified SHLi, SARi and SHRi such that they always mask the immediate value, to avoid the possibility of creating invalid or unexpected instructions.
* Added ANDri, to allow _l = _r AND _imm.
* Added tests to trace-tests.js to detect errors in the shift operation.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 368256 [details] [diff] [review]
Move extra AND instruction to NativeARM.cpp and add some trace tests to test it.

Looks good, one nit:

>--- a/js/src/nanojit/NativeARM.cpp
>+++ b/js/src/nanojit/NativeARM.cpp
>@@ -1300,8 +1300,11 @@
>     // outside of +/-255 (for AND) r outside of
>     // 0..255 for others.
>     if (!forceReg) {
>-        if (rhs->isconst() && !isU8(rhs->constval()))
>-            forceReg = true;
>+        // Shift operations are masked with 0x1f anyway, so there's no need to
>+        // force register assignment in these cases.
>+        if ((op != LIR_lsh) && (op != LIR_rsh) && (op != LIR_ush))
>+            if (rhs->isconst() && !isU8(rhs->constval()))
>+                forceReg = true;

Can you combine these two if statements into one?  Just chain the &&'s.

If you fix that up, can you upload a new patch and poke someone on #jsapi to check in (or I can do it in the next few days)?

Comment 8

[Jacob Bramley [:jbramley]]

Attachment: Synchronization with current trunk & modification of "if" coding style (as requested).

* Style changes made as requested.
 * Re-synchronized with the TM tree as the previous patch did not apply cleanly.

Comment 9

[(not currently active) Ted Mielczarek]

Needs checkin on tracemonkey.

Comment 10

[Jacob Bramley [:jbramley]]

Attachment: Re-synchronization with tree.

Comment 11

[Robert Sayre]

sigh, this rotted again. :(

I didn't know these patches were floating around. I'll get this checked in immediately if you unbitrot it.

Comment 12

[Jacob Bramley [:jbramley]]

Attachment: Re-synchronization with tree (v2).

Comment 13

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/56634a652940

Comment 14

[Andreas Gal :gal]

Great test-cases (for non-ARM platforms, too). Thanks!

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/56634a652940

Comment 16

[Bob Clary [:bc] (inactive)]

js1_8_1/trace/trace-test.js	
http://hg.mozilla.org/tracemonkey/rev/61892f57b46a

Comment 17

[Bob Clary [:bc] (inactive)]

cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <--  trace-test.js
new revision: 1.14; previous revision: 1.13

/cvsroot/mozilla/js/tests/shell.js,v  <--  shell.js

Comment 18

[Bob Clary [:bc] (inactive)]

testShiftLeft|testShiftRightLogical|testShiftRightArithmetic
v 1.9.3