On Sun, Apr 29, 2012 at 9:08 PM, Julius Baxter <[email protected]> wrote: > On Sat, Apr 28, 2012 at 10:09 AM, Julius Baxter <[email protected]> > wrote: >> On Fri, Apr 27, 2012 at 8:46 PM, R. Diez <[email protected]> wrote: >>> Hi all: >>> >>> As previously mentioned on this e-mail list, I have forked the or1ksim Test >>> Suite in order to reorganise it so that orbuild can run it not only against >>> or1ksim, but against Verilog simulations too. >>> >>> I realised today that instruction l.extws is not working as expected. I'm >>> still in the first development stages, so I might have made some silly >>> mistake, but as I'm not getting any further I have decided to ask for help >>> here. >>> >>> According to the specification, l.extws should just copy one register to >>> another on 32-bit processors. On 64-bit processors it should also extend the >>> sign bit, but that does not apply to the current implementation in ORPSoC >>> V2. >>> >>> Test case OR1KSIM/testsuite/test-code-or1k/ext/ext.S contains the following >>> line: >>> >>> CHECK_MOVE(l.extws, 0x7fffffff) >>> >>> That produces the right results under or1ksim, that is, it prints 0x7fffffff >>> before and after executing the instruction. However, >>> when running on the ORPSoC V2 Verilog model, it prints 0x7fffffff before the >>> instruction and 0xffffffff afterwards. >>> >>> I've tracked this issue down to this excerpt in file >>> ORPSOCV2/rtl/verilog/or1200/or1200_alu.v : >>> >>> `ifdef OR1200_IMPL_ALU_EXT >>> always @(alu_op or alu_op2 or a) begin >>> casez (alu_op2) >>> `OR1200_EXTHBOP_HS : extended = {{16{a[15]}},a[15:0]}; >>> `OR1200_EXTHBOP_BS : extended = {{24{a[7]}},a[7:0]}; >>> `OR1200_EXTHBOP_HZ : extended = {16'd0,a[15:0]}; >>> `OR1200_EXTHBOP_BZ : extended = {24'd0,a[7:0]}; >>> default: extended = a; // Used for l.extw instructions >>> endcase // casez (alu_op2) >>> end >>> `endif >>> >>> According to the spec, instruction l.extws has opcode 0x0 and opcode 0xd. >>> After adding $display traces, I'm getting alu_op = 0x0d and alu_op2 = 0x0 at >>> that point, which makes sense. However, the case statement only checks >>> alu_op2, and not alu_op, so it does not go down the "default:" path, but it >>> takes the OR1200_EXTHBOP_HS path. That's probably the reason why the results >>> differ. I wonder if alu_op2 should actually be alu_op there. >>> >>> I'm not familiar with those instructions or the Verilog implementation. Can >>> someone help here? >> >> Hi Ruben, >> >> It sounds like you've come across incorrect behaviour. Perhaps we >> should be decoding for the extend-word instructions, too. >> >>> >>> I've looked again at the spec, and instructions l.extws and l.extwz seem >>> unnecessary on 32-bit implementations. If you want to copy a register you >>> can just issue a "l.ori Rdest, Rsource, 0". Is that right? >> >> Yes, basically it should just be a register copy. >> >> Can't look into it in detail today, but will check it out tomorrow, >> but it looks like all we need to do is properly decode for the >> extend-word case. >> >> Cheers >> >> Julius > > OK, I've confirmed this is a bug. > > This is easily fixed with: > > Index: rtl/verilog/or1200/or1200_alu.v > =================================================================== > --- rtl/verilog/or1200/or1200_alu.v (revision 794) > +++ rtl/verilog/or1200/or1200_alu.v (working copy) > @@ -247,7 +247,7 @@ > result = extended; > end > `OR1200_ALUOP_EXTW : begin > - result = extended; > + result = a; > end > `endif
I think we should commit this fix for the incorrect behaviour. Can someone else say that they're happy for me to commit this? Julius _______________________________________________ OpenRISC mailing list [email protected] http://lists.openrisc.net/listinfo/openrisc
