Permalink
Browse files

Fix two armjit bugs the testrunner found.

  • Loading branch information...
hrydgard committed Mar 6, 2013
1 parent a0cf3b9 commit 963a6603fc3bcac58ead284367f26ece987e0dd5
Showing with 9 additions and 4 deletions.
  1. +4 −3 Core/MIPS/ARM/ArmCompALU.cpp
  2. +5 −1 android/jni/TestRunner.cpp
@@ -171,7 +171,8 @@ namespace MIPSComp
break;
case 23: //clo
gpr.MapDirtyIn(rd, rs);
RSB(R0, gpr.R(rs), Operand2(0));
MOVI2R(R0, 0xFFFFFFFF);
EOR(R0, R0, gpr.R(rs));
CLZ(gpr.R(rd), R0);
break;
default:
@@ -346,7 +347,7 @@ namespace MIPSComp
MOV(gpr.R(rd), Operand2(gpr.R(rt), shiftType, sa));
return;
}
gpr.MapDirtyInIn(rd, rs, rt, false);
gpr.MapDirtyInIn(rd, rs, rt);
AND(R0, gpr.R(rs), Operand2(0x1F));
MOV(gpr.R(rd), Operand2(gpr.R(rt), shiftType, R0));
}
@@ -363,7 +364,7 @@ namespace MIPSComp
case 2: CompShiftImm(op, rs == 1 ? ST_ROR : ST_LSR); break; //srl
case 3: CompShiftImm(op, ST_ASR); break; //sra
case 4: CompShiftVar(op, ST_LSL); break; //sllv
case 6: CompShiftVar(op, rs == 1 ? ST_ROR : ST_LSR); break; //srlv
case 6: CompShiftVar(op, fd == 1 ? ST_ROR : ST_LSR); break; //srlv
case 7: CompShiftVar(op, ST_ASR); break; //srav
default:
@@ -109,7 +109,9 @@ void RunTests()
std::istringstream logoutput(output);
int line = 0;
while (true) {
++line;
std::string e, o;
std::getline(expected, e);
std::getline(logoutput, o);
@@ -119,7 +121,9 @@ void RunTests()
while (o[o.size()-1] == 10 || o[o.size()-1] == 13)
o = o.substr(0, o.size() - 1); // For some reason we get some extra character
if (e != o) {
ELOG("DIFF! %i vs %i, %s vs %s", (int)e.size(), (int)o.size(), e.c_str(), o.c_str());
ELOG("DIFF on line %i!", line);
ILOG("O: %s", e.c_str());
ILOG("E: %s", o.c_str());
}
if (expected.eof()) {
break;

11 comments on commit 963a660

@hrydgard

This comment has been minimized.

Show comment
Hide comment
@hrydgard

hrydgard Mar 6, 2013

Owner

In addition:

tests/cpu/fpu/fpu:

03-07 00:35:39.735: E/NativeApp(6338): DIFF on line 1099!
03-07 00:35:39.735: I/NativeApp(6338): O: 2.000000
03-07 00:35:39.735: I/NativeApp(6338): E: 1.000000
03-07 00:35:39.735: E/NativeApp(6338): DIFF on line 1103!
03-07 00:35:39.735: I/NativeApp(6338): O: -1.000000
03-07 00:35:39.735: I/NativeApp(6338): E: 0.000000
03-07 00:35:39.735: E/NativeApp(6338): DIFF on line 1105!
03-07 00:35:39.735: I/NativeApp(6338): O: -1.000000
03-07 00:35:39.735: I/NativeApp(6338): E: 0.000000
03-07 00:35:39.735: E/NativeApp(6338): DIFF on line 1107!
03-07 00:35:39.735: I/NativeApp(6338): O: -1.000000
03-07 00:35:39.735: I/NativeApp(6338): E: 0.000000
03-07 00:35:39.735: E/NativeApp(6338): DIFF on line 1109!
03-07 00:35:39.735: I/NativeApp(6338): O: -2.000000
03-07 00:35:39.735: I/NativeApp(6338): E: 0.000000
03-07 00:35:39.735: E/NativeApp(6338): DIFF on line 1111!
03-07 00:35:39.745: I/NativeApp(6338): O: 2.000000
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1115!
03-07 00:35:39.745: I/NativeApp(6338): O: 2.000000
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1119!
03-07 00:35:39.745: I/NativeApp(6338): O: 2.000000
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1123!
03-07 00:35:39.745: I/NativeApp(6338): O: 4.000000
03-07 00:35:39.745: I/NativeApp(6338): E: 3.000000
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1131!
03-07 00:35:39.745: I/NativeApp(6338): O: 2.000000
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1138!
03-07 00:35:39.745: I/NativeApp(6338): O: 0.000000 0.000000 ueq: T
03-07 00:35:39.745: I/NativeApp(6338): E: 0.000000 0.000000 ueq: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1146!
03-07 00:35:39.745: I/NativeApp(6338): O: 0.000000 0.000000 ngl: T
03-07 00:35:39.745: I/NativeApp(6338): E: 0.000000 0.000000 ngl: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1155!
03-07 00:35:39.745: I/NativeApp(6338): O: 1.000000 1.000000 ueq: T
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000 1.000000 ueq: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1163!
03-07 00:35:39.745: I/NativeApp(6338): O: 1.000000 1.000000 ngl: T
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000 1.000000 ngl: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1206!
03-07 00:35:39.745: I/NativeApp(6338): O: nan 1.000000 ueq: T
03-07 00:35:39.745: I/NativeApp(6338): E: nan 1.000000 ueq: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1214!
03-07 00:35:39.745: I/NativeApp(6338): O: nan 1.000000 ngl: T
03-07 00:35:39.745: I/NativeApp(6338): E: nan 1.000000 ngl: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1240!
03-07 00:35:39.745: I/NativeApp(6338): O: 1.000000 nan ueq: T
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000 nan ueq: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1248!
03-07 00:35:39.745: I/NativeApp(6338): O: 1.000000 nan ngl: T
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000 nan ngl: F

Owner

hrydgard replied Mar 6, 2013

In addition:

tests/cpu/fpu/fpu:

03-07 00:35:39.735: E/NativeApp(6338): DIFF on line 1099!
03-07 00:35:39.735: I/NativeApp(6338): O: 2.000000
03-07 00:35:39.735: I/NativeApp(6338): E: 1.000000
03-07 00:35:39.735: E/NativeApp(6338): DIFF on line 1103!
03-07 00:35:39.735: I/NativeApp(6338): O: -1.000000
03-07 00:35:39.735: I/NativeApp(6338): E: 0.000000
03-07 00:35:39.735: E/NativeApp(6338): DIFF on line 1105!
03-07 00:35:39.735: I/NativeApp(6338): O: -1.000000
03-07 00:35:39.735: I/NativeApp(6338): E: 0.000000
03-07 00:35:39.735: E/NativeApp(6338): DIFF on line 1107!
03-07 00:35:39.735: I/NativeApp(6338): O: -1.000000
03-07 00:35:39.735: I/NativeApp(6338): E: 0.000000
03-07 00:35:39.735: E/NativeApp(6338): DIFF on line 1109!
03-07 00:35:39.735: I/NativeApp(6338): O: -2.000000
03-07 00:35:39.735: I/NativeApp(6338): E: 0.000000
03-07 00:35:39.735: E/NativeApp(6338): DIFF on line 1111!
03-07 00:35:39.745: I/NativeApp(6338): O: 2.000000
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1115!
03-07 00:35:39.745: I/NativeApp(6338): O: 2.000000
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1119!
03-07 00:35:39.745: I/NativeApp(6338): O: 2.000000
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1123!
03-07 00:35:39.745: I/NativeApp(6338): O: 4.000000
03-07 00:35:39.745: I/NativeApp(6338): E: 3.000000
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1131!
03-07 00:35:39.745: I/NativeApp(6338): O: 2.000000
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1138!
03-07 00:35:39.745: I/NativeApp(6338): O: 0.000000 0.000000 ueq: T
03-07 00:35:39.745: I/NativeApp(6338): E: 0.000000 0.000000 ueq: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1146!
03-07 00:35:39.745: I/NativeApp(6338): O: 0.000000 0.000000 ngl: T
03-07 00:35:39.745: I/NativeApp(6338): E: 0.000000 0.000000 ngl: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1155!
03-07 00:35:39.745: I/NativeApp(6338): O: 1.000000 1.000000 ueq: T
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000 1.000000 ueq: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1163!
03-07 00:35:39.745: I/NativeApp(6338): O: 1.000000 1.000000 ngl: T
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000 1.000000 ngl: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1206!
03-07 00:35:39.745: I/NativeApp(6338): O: nan 1.000000 ueq: T
03-07 00:35:39.745: I/NativeApp(6338): E: nan 1.000000 ueq: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1214!
03-07 00:35:39.745: I/NativeApp(6338): O: nan 1.000000 ngl: T
03-07 00:35:39.745: I/NativeApp(6338): E: nan 1.000000 ngl: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1240!
03-07 00:35:39.745: I/NativeApp(6338): O: 1.000000 nan ueq: T
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000 nan ueq: F
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1248!
03-07 00:35:39.745: I/NativeApp(6338): O: 1.000000 nan ngl: T
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000 nan ngl: F

@hrydgard

This comment has been minimized.

Show comment
Hide comment
@hrydgard

hrydgard Mar 6, 2013

Owner

@xsacha , should be very easy for you to add symbian support to the test runner, maybe it already works :)

Owner

hrydgard replied Mar 6, 2013

@xsacha , should be very easy for you to add symbian support to the test runner, maybe it already works :)

@xsacha

This comment has been minimized.

Show comment
Hide comment
@xsacha

xsacha Mar 7, 2013

Collaborator

Cool, thanks. I will use that from now on!

Collaborator

xsacha replied Mar 7, 2013

Cool, thanks. I will use that from now on!

@xsacha

This comment has been minimized.

Show comment
Hide comment
@xsacha

xsacha Mar 7, 2013

Collaborator

Looks like the interpreter is wrong for those conditional FPU instructions.
For instance: UEQ means 'unordered or equal' and we see 0 == 0 with expected value of 'False'.

Not sure how (0 == 0) || isnan(0) || isnan(0) worked out to be false :
It even says that ones with NaN are false. Seems to be doing opposite.

Oh wait, it gets these from pspautotests? A real device?

Collaborator

xsacha replied Mar 7, 2013

Looks like the interpreter is wrong for those conditional FPU instructions.
For instance: UEQ means 'unordered or equal' and we see 0 == 0 with expected value of 'False'.

Not sure how (0 == 0) || isnan(0) || isnan(0) worked out to be false :
It even says that ones with NaN are false. Seems to be doing opposite.

Oh wait, it gets these from pspautotests? A real device?

@unknownbrackets

This comment has been minimized.

Show comment
Hide comment
@unknownbrackets

unknownbrackets Mar 7, 2013

Collaborator

Due to a bug, the above O/E are reversed in the comment above. To keep things simple, let's look at just the expected file:

https://github.com/hrydgard/pspautotests/blob/master/tests/cpu/fpu/fpu.expected

0.000000 0.000000 ueq: T

The interpreter does pass these tests, but armjit is currently giving a false value. Currently it does:

EQ (==): 1
NEQ (!=): 0
VC (!nan): 0

I think that's wrong, because != includes the unordered case. And when they are both non nan, it will trigger the VC case.

I would do:

SetCC(CC_NEQ);
MOVI2R(R0, 0);
SetCC(CC_EQ);
MOVI2R(R0, 1);
SetCC(CC_VS);
MOVI2R(R0, 1);

// Or something...
skipMov0ToR0 = true;

Not sure if just moving 0 in and then overwriting would be faster or what, there might be better ways. But the important thing is unordered should = 1 and not unordered should not necessarily = 0.

Also note that the cpu tests are not 100% complete, e.g. they don't include lwl and such. Definitely more to add...

-[Unknown]

Collaborator

unknownbrackets replied Mar 7, 2013

Due to a bug, the above O/E are reversed in the comment above. To keep things simple, let's look at just the expected file:

https://github.com/hrydgard/pspautotests/blob/master/tests/cpu/fpu/fpu.expected

0.000000 0.000000 ueq: T

The interpreter does pass these tests, but armjit is currently giving a false value. Currently it does:

EQ (==): 1
NEQ (!=): 0
VC (!nan): 0

I think that's wrong, because != includes the unordered case. And when they are both non nan, it will trigger the VC case.

I would do:

SetCC(CC_NEQ);
MOVI2R(R0, 0);
SetCC(CC_EQ);
MOVI2R(R0, 1);
SetCC(CC_VS);
MOVI2R(R0, 1);

// Or something...
skipMov0ToR0 = true;

Not sure if just moving 0 in and then overwriting would be faster or what, there might be better ways. But the important thing is unordered should = 1 and not unordered should not necessarily = 0.

Also note that the cpu tests are not 100% complete, e.g. they don't include lwl and such. Definitely more to add...

-[Unknown]

@xsacha

This comment has been minimized.

Show comment
Hide comment
@xsacha

xsacha Mar 7, 2013

Collaborator

Oh, they are reversed. Makes sense now.
So basically SetCC(CC_VC); MOVI2R(R0,0) should be the other way around: SetCC(CC_VS); MOVI2R(R0,1); so that it covers the unordered case and not the !unordered case.
I was trying to avoid the skipping mov0 to r0 issue, heh.

By the way,the test crashes when I try to run it.
[Qt Message] Finished running test cpu/cpu_alu/cpu_alu
Thread has crashed: A data abort exception has occurred accessing 0x25a02000.

Collaborator

xsacha replied Mar 7, 2013

Oh, they are reversed. Makes sense now.
So basically SetCC(CC_VC); MOVI2R(R0,0) should be the other way around: SetCC(CC_VS); MOVI2R(R0,1); so that it covers the unordered case and not the !unordered case.
I was trying to avoid the skipping mov0 to r0 issue, heh.

By the way,the test crashes when I try to run it.
[Qt Message] Finished running test cpu/cpu_alu/cpu_alu
Thread has crashed: A data abort exception has occurred accessing 0x25a02000.

@unknownbrackets

This comment has been minimized.

Show comment
Hide comment
@unknownbrackets

unknownbrackets Mar 7, 2013

Collaborator

Hmm. I would try enabling CONDITIONAL_DISABLE everywhere and see if it still crashes... if yes -> armjit bug still.

If not, then if you have the pspsdk set up, I would try commenting out sections of the test, and try and see if you can find it that way.

-[Unknown]

Collaborator

unknownbrackets replied Mar 7, 2013

Hmm. I would try enabling CONDITIONAL_DISABLE everywhere and see if it still crashes... if yes -> armjit bug still.

If not, then if you have the pspsdk set up, I would try commenting out sections of the test, and try and see if you can find it that way.

-[Unknown]

@xsacha

This comment has been minimized.

Show comment
Hide comment
@xsacha

xsacha Mar 7, 2013

Collaborator

Ok, now the rounding issues.
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1131!
03-07 00:35:39.745: I/NativeApp(6338): O: 2.000000
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000

For, Floor(1.5f)

Edit: nevermind this one is a ceil, so 2.0 is correct. I wonder why mine gets 1.0 :(
I add 0.5f and then round to nearest.

Collaborator

xsacha replied Mar 7, 2013

Ok, now the rounding issues.
03-07 00:35:39.745: E/NativeApp(6338): DIFF on line 1131!
03-07 00:35:39.745: I/NativeApp(6338): O: 2.000000
03-07 00:35:39.745: I/NativeApp(6338): E: 1.000000

For, Floor(1.5f)

Edit: nevermind this one is a ceil, so 2.0 is correct. I wonder why mine gets 1.0 :(
I add 0.5f and then round to nearest.

@unknownbrackets

This comment has been minimized.

Show comment
Hide comment
@unknownbrackets

unknownbrackets Mar 7, 2013

Collaborator

Hmm, I'm not sure, but maybe it's using the even/odd rounding?

-[Unknown]

Collaborator

unknownbrackets replied Mar 7, 2013

Hmm, I'm not sure, but maybe it's using the even/odd rounding?

-[Unknown]

@xsacha

This comment has been minimized.

Show comment
Hide comment
@xsacha

xsacha Mar 7, 2013

Collaborator

Apparently the default FPU rounding mode is Nearest.
My Symbian platform notes say this too. Unless Android changes this somehow.

Collaborator

xsacha replied Mar 7, 2013

Apparently the default FPU rounding mode is Nearest.
My Symbian platform notes say this too. Unless Android changes this somehow.

@hrydgard

This comment has been minimized.

Show comment
Hide comment
@hrydgard

hrydgard Mar 7, 2013

Owner

Then we just need to change it? I guess we need to add a couple of ops to the emitter maybe. (mov to / from FPSCR)

Owner

hrydgard replied Mar 7, 2013

Then we just need to change it? I guess we need to add a couple of ops to the emitter maybe. (mov to / from FPSCR)

Please sign in to comment.