-
Notifications
You must be signed in to change notification settings - Fork 66
Fix/false load-use stall by gating rs1 amd rs2 hazard check #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| val usesRs2 = (opcode === InstructionTypes.RM) || | ||
| (opcode === InstructionTypes.S) || | ||
| (opcode === InstructionTypes.B) | ||
|
|
||
| val usesRs1 = !(opcode === Instructions.jal) && | ||
| !(opcode === Instructions.lui) && | ||
| !(opcode === Instructions.auipc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixed camelCase and snake_case:
val usesRs2 = ... // camelCase
io.uses_rs2_id := usesRs2 // snake_case Should be consistent - use snake_case per project conventions:
val uses_rs2 = ...
io.uses_rs2_id := uses_rs2There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request adds uses_rs1_id and uses_rs2_id signals and wires them to Control, but Control.scala still has ? placeholders. The hazard detection logic is not modified to use these signals.
Looking at the author's full implementation in their fork, the intended fix is:
val hazard_ex_rs1 = io.uses_rs1_id && (io.rd_ex === io.rs1_id)
val hazard_ex_rs2 = io.uses_rs2_id && (io.rd_ex === io.rs2_id)
when(
((io.memory_read_enable_ex || io.jump_instruction_id) &&
(io.rd_ex =/= 0.U) &&
(hazard_ex_rs1 || hazard_ex_rs2))
||
(io.jump_instruction_id &&
io.memory_read_enable_mem &&
(io.rd_mem =/= 0.U) &&
((io.uses_rs1_id && (io.rd_mem === io.rs1_id)) ||
(io.uses_rs2_id && (io.rd_mem === io.rs2_id))))
) { ... }The PR needs to include this Control.scala change or the fix is non-functional.
| val usesRs1 = !(opcode === Instructions.jal) && | ||
| !(opcode === Instructions.lui) && | ||
| !(opcode === Instructions.auipc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing edge cases:
- CSR immediate variants (csrrwi, csrrsi, csrrci) use
zimm[4:0]in rs1 field, not actual rs1 - fence instruction doesn't use rs1 for data dependency purposes
Suggested fix:
val usesRs1 = !(opcode === Instructions.jal) &&
!(opcode === Instructions.lui) &&
!(opcode === Instructions.auipc) &&
!(opcode === Instructions.fence) &&
!(opcode === Instructions.csr &&
(funct3 === InstructionsTypeCSR.csrrwi ||
funct3 === InstructionsTypeCSR.csrrsi ||
funct3 === InstructionsTypeCSR.csrrci))|
Exercise 19 Intent: This is part of CA25 Exercise 19. The PR adds the usage signals which is good guidance, but should:
Since this PR is meant to be a complete fix, the Control.scala hazard logic needs updating. |
|
Consider to contribute test case demonstrating false stall is eliminated: lw x16, 16(x0) # imm[4:0] = 16 = x16
add x1, x2, x3 # rs2 = x3, should NOT stall despite x16 match |
Fix false load-use hazards by gating rs1/rs2 usage in ID stage
Background
While analyzing control hazard waveforms, I discovered that the current load-use hazard detection logic can spuriously trigger stalls for certain instruction patterns.
Specifically, the control logic in
Control.scalacomparesrd_exagainst bothrs1_idandrs2_idunconditionally. However, for several instruction types, the bit positions corresponding tors1orrs2do not represent architectural source registers.Root Cause
For I-type load instructions (e.g.,
lw a0, 16(a5)), onlyrs1is architecturally used.rs2_idis still wired directly from instruction bits[24:20], which encodeimm[4:0]for I-type instructions.When the immediate value coincidentally equals a register index (e.g.,
imm = 16→x16), the conditionmay incorrectly evaluate to true.
This causes
io_pc_stall,io_if_stall, andio_id_flushto assert, inserting an unnecessary bubble even though no real data dependency exists.This behavior is technically consistent with the existing implementation but reflects a decode-level limitation: the hazard unit does not know whether the ID-stage instruction actually uses
rs1orrs2.Fixes in this PR
Explicit rs1 / rs2 usage signals
rs1and/orrs2jal,auipc, andluido not users1(the same bit positions encode immediates)rs2is only considered for instruction classes that architecturally use a second source register, including:add,sub,and,or)sw)beq,bne)Result
You can check the whole implementation logic in the Control.scala on my Github repo