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
_______________________________________________
OpenRISC mailing list
[email protected]
http://lists.openrisc.net/listinfo/openrisc
