535709 - nanojit: fix regstate updates for ARM

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Nicholas Nethercote [inactive]]

I won't be able to do this easily, as I only have an ARM emulator which is very sloooooow.

Comment 1

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 3

[Jacob Bramley [:jbramley]]

Attachment: Replace deprecated API usage in the ARM back-end.

Ok, here it is. This patch is from 'hg diff -r qparent -r qtip', as there are many small (almost-per-asm-function) patches which will be fiddly to upload individually. If you prefer an hg bundle or some other format, let me know.

The solution has no regressions on the lirasm test or in Trace Monkey, but I haven't yet tested in Tamarin.

Comment 4

[Jacob Bramley [:jbramley]]

Attachment: As before, but from "hg export".

This is the same but from "hg export qbase:qtip". This is probably easier to read as each mq patch is presented in order.

Comment 5

[Nicholas Nethercote [inactive]]

Oh man, I dropped the ball on this review, sorry.  I'll get to it by Monday!

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 455111 [details] [diff] [review]
As before, but from "hg export".

Thanks for doing this patch!  I hope you think the code is easier to
understand and less error-prone as a result.

Sorry I took so long to review it.  I generally try to do reviews within a
day or two so feel free to ping me if I take longer than that, because it
probably means the review slipped through the cracks.

Two thumbs up for all the extra lirasm tests.  Have you tested them on x86
as well as ARM?  I hope so.

I'm almost happy to give an r+, my main concern is the changes to the
mul_*.in tests, see below.


>diff --git a/lirasm/tests/negnot.in b/lirasm/tests/negnot.in
>new file mode 100644
>--- /dev/null
>+++ b/lirasm/tests/negnot.in
>@@ -0,0 +1,4 @@
>+i = immi 0
>+a = noti i
>+b = negi a
>+reti b
>diff --git a/lirasm/tests/negnot.out b/lirasm/tests/negnot.out
>new file mode 100644
>--- /dev/null
>+++ b/lirasm/tests/negnot.out
>@@ -0,0 +1,1 @@
>+Output is: 1

Shouldn't that result should be -1?  It's negi(noti(0)).


> void
> Assembler::asm_qjoin(LIns *ins)
> {
>     int d = findMemFor(ins);
>     NanoAssert(d);
>     LIns* lo = ins->oprnd1();
>     LIns* hi = ins->oprnd2();
> 
>-    Register r = findRegFor(hi, GpRegs);
>-    asm_str(r, FP, d+4);
>+    Register rlo = findRegFor(lo, GpRegs);
>+    Register rhi = findRegFor(hi, GpRegs & ~rmask(rlo));

You can use findRegFor2() here, it can give better results.

 
>
>diff --git a/lirasm/lirasm.cpp b/lirasm/lirasm.cpp
>--- a/lirasm/lirasm.cpp
>+++ b/lirasm/lirasm.cpp
>@@ -1461,25 +1461,39 @@ FragmentAssembler::assembleRandomFragmen
>     Q_I_ops.push_back(LIR_i2q);
>     Q_I_ops.push_back(LIR_ui2uq);
> 
>     vector<LOpcode> I_Q_ops;
>     I_Q_ops.push_back(LIR_q2i);
> #endif
> 
>     vector<LOpcode> D_I_ops;
>-    D_I_ops.push_back(LIR_i2d);
>-    D_I_ops.push_back(LIR_ui2d);
>+
>+#ifdef NANOJIT_ARM
>+    // Don't emit LIR_{ui,i}2d for soft-float variants of ARM.
>+    if (avmplus::AvmCore::config.arm_vfp)
>+#endif
>+    {
>+        D_I_ops.push_back(LIR_i2d);
>+        D_I_ops.push_back(LIR_ui2d);
>+    }

Why not?  And likewise for LIR_d2i.


> void
> Assembler::asm_condd(LIns* ins)
> {
>-    // only want certain regs
>-    Register r = deprecated_prepResultReg(ins, AllowableFlagRegs);
>+    Register rd = prepareResultReg(ins, GpRegs);
>+
>+    // TODO: Modify cmpd to allow the FP flags to move directly to an ARM
>+    // machine register, then use simple bit operations here rather than
>+    // conditional moves.
> 
>     switch (ins->opcode()) {
>-        case LIR_eqd: SETEQ(r); break;
>-        case LIR_ltd: SETLO(r); break; // } note: VFP LT/LE operations require use of
>-        case LIR_led: SETLS(r); break; // } unsigned LO/LS condition codes!
>-        case LIR_ged: SETGE(r); break;
>-        case LIR_gtd: SETGT(r); break;
>-        default: NanoAssert(0); break;
>+        case LIR_eqd:   SETEQ(rd);      break;
>+        case LIR_ltd:   SETLO(rd);      break; // } Note: VFP LT/LE operations require
>+        case LIR_led:   SETLS(rd);      break; // } unsigned LO/LS condition codes!

Nit: remove the '}' chars in the comments.



>diff --git a/lirasm/tests/mul_xyy.in b/lirasm/tests/mul_xyy.in
>--- a/lirasm/tests/mul_xyy.in
>+++ b/lirasm/tests/mul_xyy.in
>@@ -1,11 +1,9 @@
>-; 46340 * 46340 < 2^31, and will not overflow.
> big = immi 46340
> 
>-res = mulxovi big big    ; no overflow, so we don't exit here
>+res = muli big big
> 
> ; Ensure that 'big' gets its own register and isn't shared with 'res'.
>-; Also store 'res' so it isn't dead.
> m = allocp 8
> sti big m 0
>-sti res m 4
>-x                       ; we exit here
>+
>+reti res

Why did you change all these {add,sub,mul}xovi tests?  They serve a useful
purpose.

Comment 7

[Jacob Bramley [:jbramley]]

(In reply to comment #6)
> Thanks for doing this patch!  I hope you think the code is easier to
> understand and less error-prone as a result.

Oh, definitely!

> Sorry I took so long to review it.

No worries. Sorry it's taken me a while to respond; I've been concentrating on JM recently. I told Mike that I'd apply this after his ARMv4T patch (bug 552624) as there is a minor clash in one function.

> Two thumbs up for all the extra lirasm tests.  Have you tested them on x86
> as well as ARM?  I hope so.

Yep.

> Shouldn't that result should be -1?  It's negi(noti(0)).

It works if noti is a bitwise not: noti(0) becomes -1 in that case.

If that isn't correct I'll update the test, but that's how ARM, x86 and x64 implement it, at least.

> You can use findRegFor2() here, it can give better results.

Good point!

> >+#ifdef NANOJIT_ARM
> >+    // Don't emit LIR_{ui,i}2d for soft-float variants of ARM.
> >+    if (avmplus::AvmCore::config.arm_vfp)
> >+#endif
> >+    {
> >+        D_I_ops.push_back(LIR_i2d);
> >+        D_I_ops.push_back(LIR_ui2d);
> >+    }
> 
> Why not?  And likewise for LIR_d2i.

The SoftFloatFilter removes them so the back-end never encounters them in soft-float mode. I will add a comment to explain this.

> Nit: remove the '}' chars in the comments.

Will do.

> Why did you change all these {add,sub,mul}xovi tests?  They serve a useful
> purpose.

They do, and they still do. My patch changes the simple "mul_xxx" tests to use "mul" rather than "mulxovi". There are separate tests for the xovi variants ("muxov_xxx" and the like).

Actually, on reading this all again, I think I should leave the mulxovi in the "mul_xxx" test to ensure that it doesn't trigger the overflow path, and also run a normal "mul" to ensure that it calculates the result correctly.

("mul" and "mulxovi" might have different code paths, even in the case where they don't overflow.)

----

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to comment #7)
> 
> > Shouldn't that result should be -1?  It's negi(noti(0)).
> 
> It works if noti is a bitwise not: noti(0) becomes -1 in that case.

Oh, yes.

> > Why did you change all these {add,sub,mul}xovi tests?  They serve a useful
> > purpose.
> 
> They do, and they still do. My patch changes the simple "mul_xxx" tests to use
> "mul" rather than "mulxovi". There are separate tests for the xovi variants
> ("muxov_xxx" and the like).
> 
> Actually, on reading this all again, I think I should leave the mulxovi in the
> "mul_xxx" test to ensure that it doesn't trigger the overflow path, and also
> run a normal "mul" to ensure that it calculates the result correctly.

That sounds good!  Thanks.

Comment 9

[Jacob Bramley [:jbramley]]

Attachment: Update.

Highlights:
  * Rebase.
  * Shuffles the lirasm tests around, as discussed.
  * Fixes a bug in asm_stkarg.
  * Other minor tweaks.

It showed no regressions for the tests in all three repositories on ARM, x86 and x64. However, I had to tweak one or two things since testing so I will test again before pushing.

Comment 10

[Jacob Bramley [:jbramley]]

http://hg.mozilla.org/projects/nanojit-central/rev/49a8ed180ad0

Comment 11

[Brent Baker]

(In reply to comment #10)
> http://hg.mozilla.org/projects/nanojit-central/rev/49a8ed180ad0

This change is causing winmo-arm configuration in tamarin-redux to fail to compile:

repo/nanojit/NativeARM.cpp(712) : error C2065: 'fp_reg' : undeclared identifier

Comment 12

[Jacob Bramley [:jbramley]]

Hmm. Sorry about that! I wonder if there's a way to test the WinCE-ABI code paths on EABI platforms. It's easy enough in a controlled test harness but perhaps more complicated in lirasm. (I don't have any suitable environment for WinCE development.)

At a glance, the fix is to use findRegFor, as at line 684 in the EABI code-path.

I can't fix it from here but I'll look at it first thing tomorrow morning (UK time). If it's critical, back out the changeset and I'll deal with it tomorrow.

Comment 13

[Jacob Bramley [:jbramley]]

I pushed a quick fix here: http://hg.mozilla.org/projects/nanojit-central/rev/7b4347388020

I tested it on Linux (by forcibly turning off NJ_ARM_EABI). Amazingly, it passed all the lirasm tests, so presumably they don't exercise the calling conventions enough to see a difference between EABI and non-EABI variants.

Comment 14

[Nicholas Nethercote [inactive]]

(In reply to comment #13)
> 
> I tested it on Linux (by forcibly turning off NJ_ARM_EABI). Amazingly, it
> passed all the lirasm tests, so presumably they don't exercise the calling
> conventions enough to see a difference between EABI and non-EABI variants.

Yeah, the lirasm tests don't have good covereage, esp. when it comes to calls.

Comment 15

[Edwin Smith]

(In reply to comment #13)
> I pushed a quick fix here:
> http://hg.mozilla.org/projects/nanojit-central/rev/7b4347388020
> 
> I tested it on Linux (by forcibly turning off NJ_ARM_EABI). Amazingly, it
> passed all the lirasm tests, so presumably they don't exercise the calling
> conventions enough to see a difference between EABI and non-EABI variants.

The quick-fix compiles okay, but causes TR to hang when executing
on the emulators we have set up for build testing.  I haven't investigated further.

Comment 16

[Nicholas Nethercote [inactive]]

Looks like this patch broke testlirc.sh on Windows:

  ../srcdir-i686-pc-mingw32/lirasm/testlirc.sh: line 82: syntax error in conditional expression: unexpected token `('

The relevant line is this:

  if [[ $infile =~ .*/(f2i|d2i|i2d|ui2d)\.in$ ]]

Is that valid bash?  Kinda looks like Perl to me.

Comment 17

[Rick Reitmaier]

http://hg.mozilla.org/tamarin-redux/rev/6cd4a609cefc

Comment 18

[Jacob Bramley [:jbramley]]

(In reply to comment #16)
>   if [[ $infile =~ .*/(f2i|d2i|i2d|ui2d)\.in$ ]]
> 
> Is that valid bash?  Kinda looks like Perl to me.

According to Google, it's valid since Bash 3, which was released in 2004. It's possible that Cygwin's Bash (or whatever you're using) is too old. You could probably do that with glob patterns but I don't know Bash well enough to propose something off the top of my head.

Calling grep is a simpler option; does the Windows test platform have it?

It might actually be cleaner to make a hard-float-only subdirectory, as we have done for soft-float and some other machine variants.

Comment 19

[Jacob Bramley [:jbramley]]

Attachment: Remove dependency on Bash 3 regex.

Comment 20

[Brent Baker]

(In reply to comment #15)
> (In reply to comment #13)
> > I pushed a quick fix here:
> > http://hg.mozilla.org/projects/nanojit-central/rev/7b4347388020
> > 
> > I tested it on Linux (by forcibly turning off NJ_ARM_EABI). Amazingly, it
> > passed all the lirasm tests, so presumably they don't exercise the calling
> > conventions enough to see a difference between EABI and non-EABI variants.
> 
> The quick-fix compiles okay, but causes TR to hang when executing
> on the emulators we have set up for build testing.  I haven't investigated
> further.

The quick fix patch is causing a bunch of failures on arm-linux running the tamarin acceptance suite. Looks like most of the errors are situated around testing NaA, Infinity, MAX|MIN values. A sampling of failing tests are:

as3/Expressions/asOperator/asOper
ecma3/GlobalObject/e15_1_2_2_1
ecma3/Math/e15_8_2_1
ecma3/ObjectObjects/e15_2_1_1_r

Comment 21

[Nicholas Nethercote [inactive]]

Comment on attachment 475018 [details] [diff] [review]
Remove dependency on Bash 3 regex.

> # ---- Platform-specific tests and configurations. ----
> 
>+# Tests for hardware floating-point.
>+# These tests use LIR instructions which are normally removed by the soft-float
>+# filter, so soft-float targets do not need to support them.
>+for infile in "$TESTS_DIR"/hardfloat/*.in
>+do
>+    runtest $infile
>+done
>+

I found this confusing at first -- it's listed under "platform-specific tests" but there is no conditional before it.  After a while I realized that's because to get the SoftFloat ARM versions you have to specify additional arguments.  r=me with something to clarify that aspect, maybe a short comment.

Comment 22

[Jacob Bramley [:jbramley]]

(In reply to comment #21)
> I found this confusing at first -- it's listed under "platform-specific tests"
> but there is no conditional before it.  After a while I realized that's because
> to get the SoftFloat ARM versions you have to specify additional arguments. 
> r=me with something to clarify that aspect, maybe a short comment.

Fair point. MIPS might want a conditional guard. I don't know what their default build supports.

Comment 23

[Jacob Bramley [:jbramley]]

(In reply to comment #16)
> Looks like this patch broke testlirc.sh on Windows:

See bug 596923.

Comment 24

[Jacob Bramley [:jbramley]]

Fixes the Bash regex stuff: http://hg.mozilla.org/projects/nanojit-central/rev/10432e19f751

Comment 25

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/e6a9a04ed86a

Comment 26

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/fa6017442572

Comment 27

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/fa6017442572