2012/5/21 Julius Baxter <[email protected]> > 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 >
Seeing that we already have a testcase for it that passes with the fix applied, I can't see any problems with applying the patch -- Olof Kindgren ______________________________________________ ORSoC Website: www.orsoc.se Email: [email protected] ______________________________________________ FPGA, ASIC, DSP - embedded SoC design
_______________________________________________ OpenRISC mailing list [email protected] http://lists.openrisc.net/listinfo/openrisc
