OpenCores
Username:
Password:

Remember me
Register

Browse

* Amber ARM-compatible core

Issue List
More carry issues with TST #23
Closed sleary opened this issue about 9 years ago
sleary commented about 9 years ago

I can confirm that the barrel shift bug is fixed. However i found more issues (in fairness i've got things setup to hunt for differences). I think this was introduced by recent changes.

The following code shows up another carry issue. Its odd in that removing cmp or tst shows up no differences.

    AREA    |main|, CODE, READONLY
    EXPORT  _start

ORG	&1000

_start

MOV	R0,#2
MOV	R1,#1
MOV	R3,#&930

CMP     R0,#1
MOV     R1,R3
TST     R1,#&10            ; =16
	
MOVCS   R0,#1 ; Expected
MOVCC   R0,#0 ; Actual

NOP
NOP

; I use this to stop simulations... ignore.	
SWI	&FFFFFF

NOP
NOP

END

The above was compiled with ASASM.

There is the output of the code running on my A540 with ARM3

https://www.dropbox.com/sc/h1emywrhd8m4wc2/AAAkJrP_0yuTb5tX39OWSouLa

Here is the output of the ArcEM vs Amber comparison tool.

stephen@development:~/svnwork/archie/bench/tests/fuzz$ ./program/Va23_core ../../tstbug2.bin Executing % on Amber/ArcEM Comparison 56 56 Failed. errors: expected,actual,bitwise difference r0(0x00000001,0x00000000,0x00000001) r15(0x24001030,0x04001033,0x20000003)

initial: r0=0x7e1d4e3c,r1=0x06f1d01b,r2=0x1d5d2ed9,r3=0x6f6f8292,r4=0x7c6b63c9,r5=0x2f69a363,r6=0x19430192,r7=0x0de580bf,r8=0x4c467585,r9=0x41ce04b8,r10=0x44a2b75b,r11=0x0b00c412,r12=0x3119e1f5,r13=0x6b052991,r14=0x66b52125,r15=0x569d67a8,r8_firq=0x304dc14d,r9_firq=0x611dec5a,r10_firq=0x1b088c0e,r11_firq=0x147c49d9,r12_firq=0x4ab42a1b,r13_firq=0x0e0912fe,r14_firq=0x464da53c,r13_irq=0x529e0ec9,r14_irq=0x4580ab03,r13_svc=0x5c49d017,r14_svc=0x322acccd,

arcem: r0=0x00000001,r1=0x00000930,r2=0x1d5d2ed9,r3=0x00000930,r4=0x7c6b63c9,r5=0x2f69a363,r6=0x19430192,r7=0x0de580bf,r8=0x4c467585,r9=0x41ce04b8,r10=0x44a2b75b,r11=0x0b00c412,r12=0x3119e1f5,r13=0x6b052991,r14=0x66b52125,r15=0x24001030,r8_firq=0x304dc14d,r9_firq=0x611dec5a,r10_firq=0x1b088c0e,r11_firq=0x147c49d9,r12_firq=0x4ab42a1b,r13_firq=0x0e0912fe,r14_firq=0x464da53c,r13_irq=0x529e0ec9,r14_irq=0x4580ab03,r13_svc=0x5c49d017,r14_svc=0x322acccd,

amber: r0=0x00000000,r1=0x00000930,r2=0x1d5d2ed9,r3=0x00000930,r4=0x7c6b63c9,r5=0x2f69a363,r6=0x19430192,r7=0x0de580bf,r8=0x4c467585,r9=0x41ce04b8,r10=0x44a2b75b,r11=0x0b00c412,r12=0x3119e1f5,r13=0x6b052991,r14=0x66b52125,r15=0x04001033,r8_firq=0x304dc14d,r9_firq=0x611dec5a,r10_firq=0x1b088c0e,r11_firq=0x147c49d9,r12_firq=0x4ab42a1b,r13_firq=0x0e0912fe,r14_firq=0x464da53c,r13_irq=0x529e0ec9,r14_irq=0x4580ab03,r13_svc=0x5c49d017,r14_svc=0x322acccd,

sleary commented about 9 years ago

I've created a little resource for you. If you download this and compile it, in addition to the fuzzer, you get a little program that will execute a raw binary file @ 0x1000 in memory on both ArcEM and Amber simultaneously and give you a comparison of the result.

All you have to do ORG your program @ 0x1000 and you can terminate execution with SWI 0xFFFFFF.

The example above is provided in the bugs folder (although you may need to tweek it to use something other than asasm and the riscos objcopy).

To invoke it first "make" and then ./program/Va23_core <binary name>

Note it cannot be an elf it has to be raw. You also need verilator (i use debian testing version).

Not only does it do the comparison but it spits out a vcd and a disassembly of the execute path to amber.vcd and amber.dis.

Its pretty good at getting you to the heart of a problem fast. If want you can also follow the ArcEM execution path by turning DEBUG=yes for the arm project in the Make file.

Hopefully people will find this useful.

https://www.dropbox.com/s/dyhxewcyubyflfu/fuzz2.tgz?dl=0

sleary commented about 9 years ago

Yuck opencores mangled that <br/>

to execute ./program/Va23_core programname

sleary commented about 9 years ago

It looks like this was introduced by the fix for 2492 (commit 83) <br> The carry rules are more complicated than the way they are currently implemented in Amber. <br> Because Amber short circuits the barrel shifter for immediate operands it needs to calculate the correct carry when it does that. Currently it doesnt and hence the wrong carry results will be obtained.

sleary commented about 9 years ago

I'll try and make it more clear... This code needs to give a carry result that gets picked up instead of barrel shift out if the operand is immediate. <br/>

<p> assign imm32_nxt = // add 0 to Rm itype == MULT ? { 32'd0 } : // 4 x number of registers itype == MTRANS ? { mtrans_base_reg_change } : itype == BRANCH ? { offset24 } : itype == TRANS ? { offset12 } : instruction[11:8] == 4'h0 ? { 24'h0, imm8[7:0] } : instruction[11:8] == 4'h1 ? { imm8[1:0], 24'h0, imm8[7:2] } : instruction[11:8] == 4'h2 ? { imm8[3:0], 24'h0, imm8[7:4] } : instruction[11:8] == 4'h3 ? { imm8[5:0], 24'h0, imm8[7:6] } : instruction[11:8] == 4'h4 ? { imm8[7:0], 24'h0 } : instruction[11:8] == 4'h5 ? { 2'h0, imm8[7:0], 22'h0 } : instruction[11:8] == 4'h6 ? { 4'h0, imm8[7:0], 20'h0 } : instruction[11:8] == 4'h7 ? { 6'h0, imm8[7:0], 18'h0 } : instruction[11:8] == 4'h8 ? { 8'h0, imm8[7:0], 16'h0 } : instruction[11:8] == 4'h9 ? { 10'h0, imm8[7:0], 14'h0 } : instruction[11:8] == 4'ha ? { 12'h0, imm8[7:0], 12'h0 } : instruction[11:8] == 4'hb ? { 14'h0, imm8[7:0], 10'h0 } : instruction[11:8] == 4'hc ? { 16'h0, imm8[7:0], 8'h0 } : instruction[11:8] == 4'hd ? { 18'h0, imm8[7:0], 6'h0 } : instruction[11:8] == 4'he ? { 20'h0, imm8[7:0], 4'h0 } : { 22'h0, imm8[7:0], 2'h0 } ; </p>
sleary commented about 9 years ago

https://www.dropbox.com/s/2zvr5tu6lxl97mp/soak_tst.txt?dl=0

This highlights the issue with the barrel shifter short circuit. Every possible immediate is tested with 50 random register sets.

sleary commented about 9 years ago

Looking at the fuzz output i think i can simplify this with this pseudo code...

If (operand is immediate) then

if (shift is zero) 

   use carry in

else 

   use high bit of imm32_nxt

end if

end if

sleary commented about 9 years ago

I fixed it for TST... what i did was i added this in a23_execute.v

assign barrel_shift_carry_real = i_barrel_shift_data_sel == 2'd0 ? (i_imm_shift_amount == 0 ? status_bits_flags1 : i_imm3231) : barrel_shift_carry;

I then changed the u_alu instance's i_barrel_shift_carry pin from 

.i_barrel_shift_carry   ( barrel_shift_carry ),

to 

.i_barrel_shift_carry   ( barrel_shift_carry_real ),

The soak test then passes.

sleary commented about 9 years ago

Possibly a separate bug, but related... I found this in a23_decode.v

assign shift_imm = instruction11:7;

which is broken when the immediate is 0x80.

i changed it to...

assign shift_imm = {instruction11:8,1'b0};

sleary commented about 9 years ago

With the two changes below the soak tests give...

https://www.dropbox.com/s/qtsf49ejbodu2wo/soak_pass.txt?dl=0

sleary commented about 9 years ago

With the two changes below the soak tests give...

https://www.dropbox.com/s/qtsf49ejbodu2wo/soak_pass.txt?dl=0

sleary commented about 9 years ago

A better fix is to not made the change to a23_decode and use this instead...

assign barrel_shift_carry_real = i_barrel_shift_data_sel == 2'd0 ? (i_imm_shift_amount4:1 == 0 ? status_bits_flags1 : i_imm3231) : barrel_shift_carry;

sleary commented about 9 years ago

I fuzzed all ALU instructions with immediate operands. This change fixes all of them.

csantifort commented about 9 years ago

Does it fix the ANDS issue as well?

sleary commented about 9 years ago

It fixes all the immediate ones. not the register <-> register ones. I'm working on those

sleary commented about 9 years ago

The following instructions seem to need to use_carry_in...

EOR/TEQ, TST/AND, MOV, MVN

I've added carry ins to those guys and added the fix with the barrel shifter shortcut and things are looking much better again.

I also noticed a small problem with the LDM/STM fix. We need to not use the user registers on the second cycle ( control_state == MTRANS_EXEC1 ) because we end up writing back to the wrong base register if we do. So you just need to revert out the change on that cycle.

I'm putting together a base SVN clean patch with all the fixes in it.

sleary commented about 9 years ago

https://www.dropbox.com/s/u5iovqsl2avhnda/amber23_fixes.diff?dl=0

This patch fixes all open amber23 bugs.

  1. Fixes all the carry issues (verified with a fuzz - will post the fuzz result)
  2. Fixes all the rrx issues.
  3. Fixes the LDM/STM issue

Also patches the use of the wire/reg name type to itype. This is a reserved keyword on some verilog variations.

Note this patch doesnt include my Data abort fixes or the translate pin additions (Implemented as o_wb_tga).

sleary commented about 9 years ago

Here are the fuzz results...

https://www.dropbox.com/s/nwjfok2fqzh0ut1/soak_patch.txt?dl=0

Summary....

Executed: 44124768 Skipped: 32 Passed: 44124768 Failed: 0

I wouldnt swear that all the bugs are gone but i'd say this was a big step forward in confidence.

sleary commented about 9 years ago

With these fixes in (and some help with another part of my core from Till Harbaum) I was able to run Archimedes Elite nicely.

csantifort commented about 9 years ago

Code changes from Stephen added to release 87 for a23, and release 88 for a25. This fixes the issue.

csantifort closed this about 9 years ago
Assignee
No one
Labels
Bug
© copyright 1999-2018 OpenCores.org, equivalent to Oliscience, all rights reserved. OpenCores®, registered trademark.