Instruction wrongly executed after core branch about cv32e40p HOT 20 CLOSED

Thanks a lot for reporting it,

Are you using the MASTER branch? which commit are you using?

I will try to reproduce it and fix it!

Best
Davide

from cv32e40p.

@timdudu

Hi, I think the problem comes from "riscv_controller.sv":

if (
          ( (data_req_ex_i == 1'b1) && (regfile_we_ex_i == 1'b1) |
           (wb_ready_i == 1'b0) && (regfile_we_wb_i == 1'b1)
          ) &
          ( (reg_d_ex_is_reg_a_i == 1'b1) | (reg_d_ex_is_reg_b_i == 1'b1) | (reg_d_ex_is_reg_c_i == 1'b1) | (regfile_waddr_ex_i == regfile_alu_waddr_id_i))
       )
    begin
      deassert_we_o   = 1'b1;
      load_stall_o    = 1'b1;
    end

When the instruction "srli a4,a4,0x8" is in ID, it is actually invalid because the CPU can find that the branch instruction "p.bneimm a2,1,526" in EX needs to be taken. Maybe we could add another signal branch_taken_ex_i to fix this:

if (
          ( (data_req_ex_i == 1'b1) && (regfile_we_ex_i == 1'b1) |
           (wb_ready_i == 1'b0) && (regfile_we_wb_i == 1'b1)
          ) &
          ( (reg_d_ex_is_reg_a_i == 1'b1) | (reg_d_ex_is_reg_b_i == 1'b1) | (reg_d_ex_is_reg_c_i == 1'b1) | 
            (regfile_waddr_ex_i == regfile_alu_waddr_id_i)) & 
            (~branch_taken_ex_i)
       )
    begin
      deassert_we_o   = 1'b1;
      load_stall_o    = 1'b1;
    end

from cv32e40p.

What's more, if the instruction in 0x5aa wants to write data into the same register a4, such as "add a4, x0, x8", it will also lead to "load_stall_o = 1" because "regfile_waddr_ex_i == regfile_alu_waddr_id_i"

from cv32e40p.

@davideschiavone I'm using pulpinov1 branch. please try latest commit. Thanks.

from cv32e40p.

@hyf6661669 Yes, I have the same understanding on this issue. Thanks.

from cv32e40p.

@timdudu I'm using master branch and the problem is the same as yours.

from cv32e40p.

Sorry,

I cannot reproduce your issue. Here is what I have done:

I executed this piece of code on PULPissimo using the MASTER branch

1c008b54: 411c lw a5,0(a0)
1c008b56: 00173363 p.bneimm a4,1,1c008b5c
1c008b5a: 83a1 srli a5,a5,0x8
1c008b5c: 0001 nop

which is similar to yours.

a4 contains 2 so branch if not equal is actually jumping to the nop at 1C008B5C.

As you told me that the problem happens when load takes several cycles, I forced the valid to arrive 8 cycles after the grant. The grant arrives at the same cycle of the request.

Here is my trace:

267473 1c008b54 00052783 lw x15, 0(x10) x15=1c008080 x10:1a104004 PA:1a104004
267474 1c008b56 00173363 p.bneimm x14, 1, 6 x14:00000002
267485 1c008b5c 00000013 nop

If you do a git diff between the master and pulpino you may find the reason of this bug as we had already in the past a similar problem that may have not been backported to pulpino.

@hyf6661669 when did you try it with the master branch?

Thanks a lot
Davide

from cv32e40p.

@davideschiavone I'm using the latest master branch. But at present my ModelSim is broken so I can't do simulations.

Can you show the wave under this situation?

from cv32e40p.

Sure, here you see the DATA BUS transaction (in RED GRANT and VALID) and the branch decision taken in the yellow with the instruction address which points to the NOP instruction.
In the yellow cursor, the instruction in the ID stage is indeed the SRLI and both load_stall_o and deassert_we_o of the controller go to '1'.

from cv32e40p.

@davideschiavone
@timdudu
I think this problem is just a misunderstanding. In fact, ri5cy is not allowed to excute later instructions if a load/store instruction doesn't get asserted data_rvalid_i, even if the rs1, rs2, rd of the load instruction has nothing to do with that of later instructions.

Timdudu may think that the core is paused because of load_stall_o = 1. Actually it is paused because the instruction lw a4,772(a4) doesn't receive asserted data_rvalid_i.

from cv32e40p.

Ok so shall you guys close the issue?

from cv32e40p.

@hyf6661669
I don't think this is a misunderstanding. You said "ri5cy is not allowed to excute later instructions if a load/store instruction doesn't get asserted data_rvalid_i, even if the rs1, rs2, rd of the load instruction has nothing to do with that of later instructions." But actually, in my test, 5a6 is executed before getting data_rvalid_i and 5a8 is executed when get data_rvalid_i.

from cv32e40p.

@davideschiavone
"In the yellow cursor, the instruction in the ID stage is indeed the SRLI and both load_stall_o and deassert_we_o of the controller go to '1'." In my test, load_stall_o and deassert_we_o are also go to 1, but when get data_rvalid_i, this two signals go to 0 and the result of SRLI is written to register.

from cv32e40p.

@timdudu
Sorry for my wrong words... In fact ,the instruciton after Load/Store is able to enter EX if Load/Store gets asserted data_gnt_i, before the Load/Store instruction does get asserted data_rvalid_i.

Actually in riscv_ex_stage.sv, we can see:

assign ex_ready_o = (~apu_stall & alu_ready & mult_ready & lsu_ready_ex_i & wb_ready_i & ~wb_contention) | (branch_in_ex_i);

And riscv_ex_stage.lsu_ready_ex_i = 1 means that riscv_load_store_unit.data_gnt_i = 1. But at present, riscv_load_store_unit.CS = IDLE, so riscv_load_store_unit.lsu_ready_wb_o = riscv_ex_stage.wb_ready_i = 1, and riscv_ex_stage.ex_ready_o = 1. In your example, it allows the instruction p.bneimm a2,1,526 to enter EX.

from cv32e40p.

@hyf6661669
So do you agree it is an issue?

from cv32e40p.

@timdudu
Yes, from your figure it seems that the instruction srli a4,a4,0x8 writes 0x0000_0101 into register x14, which is not expected.

from cv32e40p.

@davideschiavone
Can you have a look?

from cv32e40p.

@davideschiavone
"In the yellow cursor, the instruction in the ID stage is indeed the SRLI and both load_stall_o and deassert_we_o of the controller go to '1'." In my test, load_stall_o and deassert_we_o are also go to 1, but when get data_rvalid_i, this two signals go to 0 and the result of SRLI is written to register.

This looks pretty much the same I tried to reproduce but it works for me. Can I see what happens to the branch? You can have a look at my waveforms.

Also, can you please try the master branch? fixes and development have been implemented in the master, so maybe I can't see the bug because it has been fixed in the master branch.

Best
Davide

from cv32e40p.

@davideschiavone
This bug has been fixed in the master branch. Thanks.
2a8e0bb

from cv32e40p.

I'm closing this as you have reported it to be fixed.

from cv32e40p.