Updating Branch Predictors to replace hash-map with Fetch Target Queue #401
Conversation
ABenC377
requested review from
FinnWilkinson,
dANW34V3R,
jj16791 and
JosephMoore25
and removed request for
FinnWilkinson
ABenC377
added
enhancement
There was a problem hiding this comment.
I have some reservations:
My fear here is that we are starting to stray from the very generic functionality of a BranchPredictor. We add the none generic addToFTQ function that may not be required by all future predictors. Update and other functions now need to be called in a very specific order which mandates where they can be called in the pipeline. This may not play well with predictors that don't have structures like an FTQ and also means it can't get early prediction feedback i.e. update called at commit and not at the end of exec unit which could be a huge delay for feedback e.g. up to 630 entries in an M1 ROB (commit width of, say, 8 means best case delay of 79 cycles!!!) . However, I understand that to have this FTQ functionality all of this is required so I'm not sure what a good solution is to keep predictors generic but also allow more tightly integrated behaviour.
We also now need to be REALLY REALLY careful about the order in which we flush the predictor now. Even changing the order of units being flushed in the core.flush function will break the BPs logically (potentially), but will be really hard to see/find. Potentially we need to add some debug behaviour to ensure that we ALWAYS pop the correct entry e.g. only in debug mode, each entry in the FTQ also stores the associated address and there are assertions to ensure that flush addresses match up. I just think this behaviour in the current implementation is too easy to break and too subtle to realise that you have broken it.
Regardless I think we should move all predictors into their own directory now that we have a good selection.
There was a problem hiding this comment.
Great work and nearly there. A couple minor points, but these shouldn't take too long.
Please note that my comment about mask creation using the wrong types/overflowing impacts a few areas in the code, but was difficult to tell when I'm not certain on the types each variable uses. This shouldn't take too long to check, but likely faster for you than me if you're familiar with the types you set things to.
Will be nice to get this one merged :)
ABenC377
dismissed
FinnWilkinson’s stale review
The merge-base changed after approval.
ABenC377
dismissed
dANW34V3R’s stale review
The merge-base changed after approval.
ABenC377
dismissed
JosephMoore25’s stale review
The merge-base changed after approval.
ABenC377
dismissed stale reviews from FinnWilkinson and dANW34V3R
The merge-base changed after approval.
| * branch instruction, flushes them. | ||
| */ | ||
| void flushBranchesInBufferFromSelf( | ||
| pipeline::PipelineBuffer<std::shared_ptr<Instruction>>* buffer) { |
There was a problem hiding this comment.
Unless I'm mistaken, this is a pointer not a reference. For a reference, you'd need to swap * with & and pass in the buffer param in as normal.
ABenC377
dismissed
JosephMoore25’s stale review
The merge-base changed after approval.
ABenC377
dismissed
jj16791’s stale review
The merge-base changed after approval.
This update to the branch predictors (both Perceptron and Generic) replaces the hashmap that was previously being used to keep previous-state information between predict and update with a queue (called a Fetch Target Queue or FTQ) of previous-state information for the in-flight branches. The FTQ allows speculative updating of the global history (which is also included in this PR).
In terms of performance, it seems to be quite lateral. a full breakdown of stats across our benchmark suite is provided below. To summarise the results, most benchmarks have an improved misprediction rate with the update, but the changes in execution time are hit and miss with many becoming a bit slower (probably due to an increased number of interactions with the branch predictor through the pipeline).
I think this is still a worthwhile change to make to SimEng even though there isn't a clear-cut performance upgrade because it makes the flow of branches through the branch predictor more regular and so should facilitate more complicated branch predictors in the future (e.g., TAGE).