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

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.

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.

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

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

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.

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

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

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.

(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

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.

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.

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

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

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.

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

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

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

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

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