Tage #443
Tage #443
Conversation
Rebasing
|
Could you also add tage to the a64fx_SME.yaml config |
… fixed but dynamically chosen size
….e., is now different from the first tagged table)
Merging with changes to dev
Merging with dev
Merging with dev
| if (i == 0) { | ||
| history_[i] |= ((isTaken) ? 1 : 0); | ||
| } else { | ||
| history_[i] |= (((history_[i - 1] & (1ull << 63)) > 0) ? 1 : 0); |
There was a problem hiding this comment.
Does this need the conditional statement? After doing the AND you could shift right by 63 to get your 0 or 1. Would be slightly fewer cycles and more understandable/readable in my eyes (you may disagree)
There was a problem hiding this comment.
I think the conditional is needed here. Whats being loaded into the uint64 depends on where it is in the vector. All but the least-significant uint64s get the MSB of the next uint64 added as the LSB. But the least-significant uint64 gets isTaken added as the LSB. However, if I'm misunderstanding your Q LMK.
| * outcome, 'position' would be 0. | ||
| * */ | ||
| void updateHistory(bool isTaken, uint64_t position) { | ||
| if (position < size_) { |
There was a problem hiding this comment.
Should we assert position being < size_ as above, or are there cases where this could "validly" be greater? For instance, if you are trying to update an entry that has been lost from the history because there have been too many branches in the meantime?
There was a problem hiding this comment.
Exactly as you say, I don't think that this should be an assert as the core may validly try to update a history that is no longer being tracked. The reason that we should allow this is to allow the pipeline not to need to know the size of the branch history. We're already ensuring that this doesn't cause problems with our if statement on 82.
| * access and manipulate large branch histories, as are needed in | ||
| * sophisticated branch predictors. | ||
| * | ||
| * The bits of the branch history are stored in a vector of uint64_t values, |
There was a problem hiding this comment.
"vector" should be "array"
| std::vector<std::pair<uint8_t, uint64_t>> btb_; | ||
|
|
||
| /** The bitlength of the Tagged tables' indices. | ||
| * Each tagged table with have 2^bits entries. */ |
|
|
||
| uint64_t TagePredictor::getTag(uint64_t address, uint8_t table) { | ||
| // Hash function here is pretty arbitrary | ||
| uint64_t h1 = address; |
There was a problem hiding this comment.
Any reason not to remove the 2 LSBs here?
There was a problem hiding this comment.
Yes. Ideally the hashes for the tag and the index should never each produce the same value for two different branches. Therefore, because the index does remove the 2 LSBs, keeping them here makes the information being passed into the hashes different and so improves the accuracy of the BP (reduces the risk of this type of accidental clashing).
| Only needed for a ``Tage`` predictor. The number of tagged tables used by the predictor, in addition to a default prediction table (i.e., the BTB). Therefore, a value of 3 for ``Num-Tage-Tables`` would result in four total prediction tables: one BTB and three tagged tables. If no tagged tables are desired, it is recommended to use the ``GenericPredictor`` instead. | ||
|
|
||
| Tage-Length | ||
| Only needed for a ``Tage`` predictor. The number of bits used to tage the entries of the tagged tables. |
There was a problem hiding this comment.
Is the "tage" in the latter sentence meant to be that or rather "tag"
Adding a TAGE branch predictor.
Performance relative to previous best (Perceptron) summarised below: