mortbopet
commented
1 year ago Awesome for taking a stab at this, can't wait to try it out! Its a big PR so let's make sure that we take our time and get it right - given the size I'll be reviewing it in stages.
My first major comment before going into the nitty gritty details is that I think it would be elegant if the branch predictor work is detached from the notion that it's a RISCV processor that it's executing under. This would mean that checks for risc-v specific branch-like instructions aren't done inside the branch predictor tab/whereever this is done. Instead, processors should announce whether they support a branch predictor interface, and said interface should communicate ISA-agnostic information. I imagine it should be fairly simple to come up with an ISA-agnostic interface for this (i.e. "Is the current instruction a branch-like instruction? what is the branch target? branch taken, not taken, ...). I think this will both improve the code, as well as make it easier for people in the future to hook into your (now more generic) branch predictor (e.g. there are people currently who's working on a MIPS processor for Ripes, wherein it would be cool if we could just hook it directly into this branch predictor!).
In line with the above, i'd also like to see the branch predictors themselves conform to some form of interface which each predictor type inherits. This will clean up your code a lot (i.e. a separate class for TwoState, AlwaysTaken, ...) + make it easier for people to add their own predictors down the road.
Third, do you think it is possible that we can do some form of inheritance between e.g. rv5s and rv5s_br? I've had to deal with so many errors regarding tiny errors when connecting up all of the components at the top-level, so it would make good sense to me if we factor out the commonalities into a shared base model.
Anglo-software
Implemented a 5-stage branch prediction processor, along with a tab to control its properties like predictor type and whatnot, along with statistics and visualizations of internal state. Once the processor with branch prediction is selected through the processor selection menu, the branch prediction tab is enabled. The processor will hold its internal state through different program changes and reloads, allowing multiple iterations of running a single program or seeing how different programs interact through the predictor. The user can reset the state at any time if they wish. It supports reversing execution, RV32IMC, and RV64IMC.
Small note: the test programs (such as tst_riscv) supplied with Ripes do not work with the processor, but I did run each test assembly file manually and they all were successful. The errors (usually segmentation faults) originate from different places at different times, sometimes consistent between runs and sometimes producing different errors. Possible race condition?
However, in the Ripes software itself, it is very stable and error free, at least as far as I've been able to test.
If there's any changes that need to be made, I'd be more than glad to do them, and any guidance on the aforementioned issue would be greatly appreciated.