Refactor ALU to use SystemVerilog enums and fix branch logic#200
Refactor ALU to use SystemVerilog enums and fix branch logic#200TheDeepestSpace merged 6 commits intoUTOSS:mainfrom
Conversation
|
Hey @RexPaster i'll try to figure out the best way to get the CI working for you this week since you are on a fork |
| if (alu_result == 32'b1) begin | ||
| pc_src = PC_SRC__JUMP; | ||
| PCUpdate = 1'b1; | ||
| end | ||
| else pc_src = PC_SRC__INCREMENT; | ||
| case (funct3) | ||
| 3'b100, 3'b110: begin // BLT, BLTU: branch if rs1 < rs2 | ||
| if (alu_result) begin // Direct SLT/SLTU result | ||
| pc_src = PC_SRC__JUMP; | ||
| PCUpdate = 1'b1; | ||
| end | ||
| else pc_src = PC_SRC__INCREMENT; | ||
| end | ||
| 3'b101, 3'b111: begin // BGE, BGEU: branch if rs1 >= rs2 (invert SLT/SLTU) | ||
| if (!alu_result) begin // Invert SLT/SLTU result for >= comparison | ||
| pc_src = PC_SRC__JUMP; | ||
| PCUpdate = 1'b1; | ||
| end | ||
| else pc_src = PC_SRC__INCREMENT; | ||
| end | ||
| default: pc_src = PC_SRC__INCREMENT; | ||
| endcase |
There was a problem hiding this comment.
Gonna need to triple-check this since I think we were passing the base ISA tests before for the branch instructions, but might have missed something.
There was a problem hiding this comment.
OK. Originally the ALUDECODER was calling ALU functions that didn't even exist in the ALU parameters.sv section. It's possible that these were defined directly in the ALU without being defined as parameters in Parameters.sv, I'll check later today.
There was a problem hiding this comment.
I just checked and the original process before implementing this provided unknown codes to the ALU, triggering the DEFAULT case condition.
There was a problem hiding this comment.
Hey sorry for radio silence there, i've been bit busy, getting back to this now.
So im pretty sure what's happening here is that you basically merged the ALU paths for SGE/SGEU with SLT/SLTU, and in order to differentiate between BGE/BGEU and BLT/BLTU you need add this condition at the control FSM level.
And that is probably the reason why is was working fine before -- since we differentiated SGE and SLT on ALU level, even though it was technically not necessary since RISC-V does not "define" the opposite of SLT on spec-level.
I think this is a good improvement on your part here, probably would take up a bit less space, though I haven't actually checked this.
Perfect! Let me know. |
Would it be better if I did it as a branch of this Repository? |
Hey i changed up some of the ci settings so PRs from forks should be able to run CI no problem now; can you rebase your PR onto latest main or merge main into your PR? |
…e used. Update respective file references. Adjust ALU logic to use enumerated type. TODO: integrate ENUM into ALU_decoder.sv
…LT/SLTU results | Implement enum in ALUdecoder.sv
RexPaster
force-pushed
the
alu-enumeration
branch
from
1dbeb8e to
b86f843
Compare
|
@TheDeepestSpace Done! I had to look up what a rebase is but I think i did it correctly. |
|
Nice! looks like you did it right, and CI is running too now. Lemme know if you can see the outputs/commands it runs (you can also see the command in |
|
Give me a bit to fix these errors! |
|
No rush at all! |
|
@TheDeepestSpace Lets try that! |
|
I forgot to implement the changes in src/utoss_riscv.sv to switch it to enum. Im doing it now! |
|
ok! theoretically this should work. It passes svlint and the docker build! |
|
Horray! |
|
Thanks for the PR @RexPaster! |
2 participants
PR: Refactor ALU Decoder and Update Branch Comparison Logic
Closes #175
Summary
Refactor the
ALUdecoderto use thealu_op_t_lowenum instead of raw4-bit control values and update branch comparison handling.
Changes
values (
ALUAdd,ALUSub,ALUSLT, etc.).BGEandBGEUusing invertedSLT/SLTUresultsinstead of introducing new ALU operations.
Notes
Branch comparisons now follow standard RISC-V semantics:
The inversion is handled in the control logic rather than the ALU to
keep the ALU implementation generic.