Conversation
Change-Id: I44b6175022a3a593ed385407bd95ec0c40c74642
Change-Id: I16ce79a7d8488d9a138d0a26f5500576ae54132e
Change-Id: I8be037a1cdacd7151f4fe8e743a32cca90a85036
Change-Id: I6b9c92c18c574a12c19532ae0b894e64c1187342
Change-Id: I953965b8e6feb1c15a238ac832d65bc16b32f496
Change-Id: I39cd471d452aa343c3dd741a80fdfa7d126e3a9f
Change-Id: I3567ae93652aac218c5b4646003abadddaf7cf32
Change-Id: If7c9d3aa68a23c36dde74d8cf3a286c9c48f3e3c
Change-Id: I9b85fdca4602fdc28521250daec6b397361cd444
Change-Id: I886f0f6596105ea56ff167d406879277c751802c
Change-Id: Ifb354f91da3df909bcac50ebb5c683ea4c718bf1
Change-Id: I8c3e752daaa188deef4a6bb78abeb5c02a0aaca9
Change-Id: Ib9daaa3a60777c83fed779dd0169d17002a44d43
Change-Id: Ia5934e526e8d6bea03bd0711eb9cf6947eecc2aa
Change-Id: Ie3f6b39d8f6e989018fc477a7b55d3879657338b
Change-Id: I31aa12e8e2f6dd2b5421f9c4deab11dddcb53eda
Change-Id: I6c27be3fbd0db5110e0b1948ac3c39c1400b9b64
Change-Id: I21054d48efddcff13308e1164109dfbbc2d034e2
Change-Id: I84d82d8499186909ee5f79e398b8a9ff07d5bb72
Change-Id: Ie92f7106b798596ba7b0ed86cc9346fa3e0f9c38
Change-Id: Id6d0cea61bda99e19f3e6bee63e86797b98f4149
Change-Id: I16ce79a7d8488d9a138d0a26f5500576ae54132e
Change-Id: I31aa12e8e2f6dd2b5421f9c4deab11dddcb53eda
Change-Id: I6c27be3fbd0db5110e0b1948ac3c39c1400b9b64
Change-Id: I21054d48efddcff13308e1164109dfbbc2d034e2
Change-Id: I84d82d8499186909ee5f79e398b8a9ff07d5bb72
Change-Id: I914d05c0a29ce57989ba63bf2b46595c62cfcc0a
Change-Id: Id4e6f99f3399aef5d06aa8abb7eb2e118632e4ea
Change-Id: Iba1c0e49894e5662f12316054af294a52f338a06
📝 WalkthroughWalkthroughAdds a full MicroTAGE BTB predictor (header + implementation), exposes it to the Python predictor API and build system, updates DecoupledBPUWithBTB to reference MicroTAGE, adjusts BTBTAGE/UBTB defaults, and enables MicroTAGE in the kmhv3 example config. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,220,255,0.5)
participant Fetch as Fetch/BTB
end
rect rgba(200,255,200,0.5)
participant Micro as MicroTAGE
end
rect rgba(255,230,200,0.5)
participant Hist as HistoryMgmt
end
rect rgba(255,200,200,0.5)
participant Tables as TableLookup
end
Fetch->>Micro: putPCHistory(startPC, history)
Micro->>Hist: fold history / snapshot
Hist-->>Micro: folded histories
Micro->>Tables: generateSinglePrediction(btb_entry, startPC, folded_hist)
loop probe tables
Tables->>Tables: compute index/tag & match
alt tag match
Tables-->>Micro: main/alt prediction
end
end
Micro-->>Fetch: return prediction (taken, target, meta)
sequenceDiagram
rect rgba(220,220,255,0.5)
participant Commit as Commit/Resolution
participant Micro as MicroTAGE
participant Update as UpdateLogic
participant Tables as TableUpdate
end
Commit->>Micro: update(FetchStream with outcomes)
Micro->>Micro: canResolveUpdate() (bank/conflict check)
alt conflict
Micro-->>Commit: defer update
else no conflict
Micro->>Update: doResolveUpdate()
Update->>Micro: prepareUpdateEntries()
Micro->>Update: updatePredictorStateAndCheckAllocation()
alt allocation needed
Update->>Tables: handleNewEntryAllocation()
end
loop per-entry
Tables->>Tables: update counters/useful bits/LRU
end
Update-->>Micro: updates applied
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/cpu/pred/BranchPredictor.py`:
- Around line 1063-1088: The MicroTAGE class has mismatched parameter counts:
numPredictors is 1 but vector params tableSizes, TTagBitSizes, TTagPcShifts, and
histLengths have 2 elements, which can trigger constructor asserts; fix by
making these consistent—either set numPredictors = 2 to match the two-element
vectors or shrink each VectorParam (tableSizes, TTagBitSizes, TTagPcShifts,
histLengths) to a single-element list to match numPredictors = 1; update the
explanatory comment ("Keep vector parameters...") accordingly so numPredictors
and all vector lengths are always aligned.
🧹 Nitpick comments (5)
src/cpu/pred/btb/microtage.hh (1)
382-390: Consider usingstructinstead oftypedef structfor C++ idiom.The
typedef struct TageMeta { ... } TageMeta;pattern is a C idiom. In C++,struct TageMeta { ... };is sufficient and more idiomatic.✨ Suggested refactor
- typedef struct TageMeta + struct TageMeta { std::unordered_map<Addr, TagePrediction> preds; std::vector<PathFoldedHist> tagFoldedHist; std::vector<PathFoldedHist> indexFoldedHist; std::vector<PathFoldedHist> altTagFoldedHist; bitset history; // for viewing TageMeta() {} - } TageMeta; + };src/cpu/pred/btb/microtage.cc (4)
95-99:tableTagBits.resize()may overwrite values set from parameters.On line 70,
tableTagBitsis initialized fromp.TTagBitSizes. Then on line 98,tableTagBits.resize(numPredictors)is called, which could truncate the vector if it had more elements, or extend it with default values (0). This seems intentional as a safety measure, but the comment on line 119 (assert(tableTagBits.size() >= numPredictors)) suggests the expectation is that the params provide enough elements.Since you want exactly
numPredictorsentries, consider keeping the resize but document the intent, or remove it if the params are expected to be correctly sized.
282-290: Consider documenting the dry run purpose.The
dryRunCyclemethod setslastPredBankIdandpredBankValidbut has a comment saying "No operation". The bank tracking is a meaningful operation for conflict detection. Consider updating the comment to reflect the actual purpose.📝 Suggested documentation update
void MicroTAGE::dryRunCycle(Addr startPC) { - // No operation in dry run cycle for MicroTAGE // Record prediction bank for next tick's conflict detection lastPredBankId = getBankId(startPC); predBankValid = true; - - return; }
520-532: Consider const-correctness for the weakish check.Minor: The
weakishvariable could use a more descriptive name likeisWeakCounterfor clarity.
1074-1074: Spacing inconsistency afterelse.Minor formatting: The
else{on line 1074 should have a space before the brace for consistency with the rest of the codebase.✨ Suggested fix
- }else{ + } else {
| class MicroTAGE(TimedBaseBTBPredictor): | ||
| """Micro-sized BTB TAGE predictor used alongside uBTB""" | ||
| type = 'MicroTAGE' | ||
| cxx_class = 'gem5::branch_prediction::btb_pred::MicroTAGE' | ||
| cxx_header = "cpu/pred/btb/microtage.hh" | ||
|
|
||
| needMoreHistories = Param.Bool(True, "MicroTAGE needs more histories") | ||
| updateOnRead = Param.Bool(False,"Enable update on read, no need to save tage meta in FTQ") | ||
| # Keep vector parameters consistent with numPredictors to avoid constructor asserts. | ||
| numPredictors = Param.Unsigned(1, "Number of TAGE predictors") | ||
| tableSizes = VectorParam.Unsigned([512]*2,"the TAGE T0~Tn length") | ||
| TTagBitSizes = VectorParam.Unsigned([16]*2 ,"the T0~Tn entry's tag bit size") | ||
| TTagPcShifts = VectorParam.Unsigned([1]*2 ,"when the T0~Tn entry's tag generating, PC right shift") | ||
| blockSize = Param.Unsigned(32,"tage index function uses 32B aligned block address") | ||
|
|
||
| histLengths = VectorParam.Unsigned([9,18],"the BTB TAGE T0~Tn history length") | ||
| maxHistLen = Param.Unsigned(970,"The length of history passed from DBP") | ||
| numTablesToAlloc = Param.Unsigned(1,"The number of table to allocated each time") | ||
| numWays = Param.Unsigned(1, "Number of ways per set") | ||
| baseTableSize = Param.Unsigned(256,"Base table size") | ||
| maxBranchPositions = Param.Unsigned(32,"Maximum branch positions per 64-byte block") | ||
| useAltOnNaSize = Param.Unsigned(128,"Size of the useAltOnNa table") | ||
| useAltOnNaWidth = Param.Unsigned(7,"Width of the useAltOnNa table") | ||
| numBanks = Param.Unsigned(4,"Number of banks for bank conflict simulation") | ||
| enableBankConflict = Param.Bool(True,"Enable bank conflict simulation") | ||
| numDelay = Param.Unsigned(0,"Prediction latency in cycles") |
There was a problem hiding this comment.
Parameter count mismatch: numPredictors=1 but vector parameters have 2 elements.
The comment on line 1071 states "Keep vector parameters consistent with numPredictors to avoid constructor asserts", but:
numPredictors = 1tableSizes = [512]*2(2 elements)TTagBitSizes = [16]*2(2 elements)TTagPcShifts = [1]*2(2 elements)histLengths = [9,18](2 elements)
This inconsistency may cause unexpected behavior. Either increase numPredictors to 2 or reduce the vector sizes to 1 element.
🔧 Suggested fix (Option A: increase numPredictors)
- numPredictors = Param.Unsigned(1, "Number of TAGE predictors")
- tableSizes = VectorParam.Unsigned([512]*2,"the TAGE T0~Tn length")
+ numPredictors = Param.Unsigned(2, "Number of TAGE predictors")
+ tableSizes = VectorParam.Unsigned([512]*2,"the TAGE T0~Tn length")🔧 Suggested fix (Option B: reduce vector sizes)
- tableSizes = VectorParam.Unsigned([512]*2,"the TAGE T0~Tn length")
- TTagBitSizes = VectorParam.Unsigned([16]*2 ,"the T0~Tn entry's tag bit size")
- TTagPcShifts = VectorParam.Unsigned([1]*2 ,"when the T0~Tn entry's tag generating, PC right shift")
+ tableSizes = VectorParam.Unsigned([512],"the TAGE T0~Tn length")
+ TTagBitSizes = VectorParam.Unsigned([16],"the T0~Tn entry's tag bit size")
+ TTagPcShifts = VectorParam.Unsigned([1],"when the T0~Tn entry's tag generating, PC right shift")
...
- histLengths = VectorParam.Unsigned([9,18],"the BTB TAGE T0~Tn history length")
+ histLengths = VectorParam.Unsigned([9],"the BTB TAGE T0~Tn history length")🧰 Tools
🪛 Ruff (0.14.14)
[error] 1069-1069: Param may be undefined, or defined from star imports
(F405)
[error] 1070-1070: Param may be undefined, or defined from star imports
(F405)
[error] 1072-1072: Param may be undefined, or defined from star imports
(F405)
[error] 1073-1073: VectorParam may be undefined, or defined from star imports
(F405)
[error] 1074-1074: VectorParam may be undefined, or defined from star imports
(F405)
[error] 1075-1075: VectorParam may be undefined, or defined from star imports
(F405)
[error] 1076-1076: Param may be undefined, or defined from star imports
(F405)
[error] 1078-1078: VectorParam may be undefined, or defined from star imports
(F405)
[error] 1079-1079: Param may be undefined, or defined from star imports
(F405)
[error] 1080-1080: Param may be undefined, or defined from star imports
(F405)
[error] 1081-1081: Param may be undefined, or defined from star imports
(F405)
[error] 1082-1082: Param may be undefined, or defined from star imports
(F405)
[error] 1083-1083: Param may be undefined, or defined from star imports
(F405)
[error] 1084-1084: Param may be undefined, or defined from star imports
(F405)
[error] 1085-1085: Param may be undefined, or defined from star imports
(F405)
[error] 1086-1086: Param may be undefined, or defined from star imports
(F405)
[error] 1087-1087: Param may be undefined, or defined from star imports
(F405)
[error] 1088-1088: Param may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
In `@src/cpu/pred/BranchPredictor.py` around lines 1063 - 1088, The MicroTAGE
class has mismatched parameter counts: numPredictors is 1 but vector params
tableSizes, TTagBitSizes, TTagPcShifts, and histLengths have 2 elements, which
can trigger constructor asserts; fix by making these consistent—either set
numPredictors = 2 to match the two-element vectors or shrink each VectorParam
(tableSizes, TTagBitSizes, TTagPcShifts, histLengths) to a single-element list
to match numPredictors = 1; update the explanatory comment ("Keep vector
parameters...") accordingly so numPredictors and all vector lengths are always
aligned.
Change-Id: I29ba0aa7100f33faa5622f1f38d5796df34e9e8c
Change-Id: I790fb52f6fd47686fd8bf422ed27b750dd6792de
Change-Id: Ie287b2dd1b8abcf06b60274c09705aef2595d91f
Change-Id: I20b543ec370547e9dded5c68e1b4b16746a3ebb8
Change-Id: I12124577aa2b245b9136fbf865c0ea89dce53154
Change-Id: I441caa41d783668943692266293b3c65b553f31f
Change-Id: I398fe825f8f9b6477615307be253e938abcfd90c
Change-Id: I4d383b7343357e92aa17b2b25908fa357abf719f
Change-Id: If2050d097ac4674eba3c6b02db5b821048295b27
Change-Id: Ib527e252fadd979cb1670513c66c6e4bac6b1422
Change-Id: I6c7922b7cc16479c77eab66e0e870cbd6a232986
Change-Id: I0ec6da95805b5bd1b9140ff6b127477241b05c70
Change-Id: I2a4abfdb9978bd9ea6022c1438703e64a1ff8bea
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
Change-Id: I31b850b7b6a13ddff4920eb696acc120743a8fbd
Change-Id: I8b845fb157171f43d4253fed59a85efa91e8f35b
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
Summary by CodeRabbit
New Features
Chores
Bug Fixes