Conversation
Change-Id: I8c650c9cc5382b16eafce8edd0916a25a60dd688
Change-Id: I1a0e3993d6f297cf2af6ad8c45ffcef641216dec
Change-Id: I407ce281dacd047fe9975e6248a4f2a235d198d3
Change-Id: I54a18ad85ec12aed564f3a12184c4d1dba244ae4
Change-Id: Ieb052b87c6479122f89454d1c6f69fa7ef5f6f8c
Change-Id: I255b13114d984cf2e53cd003376c76fb93534aa3
📝 WalkthroughWalkthroughThis PR introduces a configurable base prediction source in BTBTAGE and MBTB components via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cpu/pred/btb/stream_struct.hh (1)
95-98: Missing initialization ofisDirectin default constructor.The default constructor initializes
isCond,isIndirect,isCall, andisReturntofalse, butisDirectis not initialized. This leaves it with an indeterminate value when using a default-constructedBranchInfo.🔎 Proposed fix
BranchInfo() - : pc(0), target(0), resolved(false), isCond(false), isIndirect(false), isCall(false), isReturn(false), size(0) + : pc(0), target(0), resolved(false), isCond(false), isIndirect(false), isDirect(false), isCall(false), isReturn(false), size(0) { }
🧹 Nitpick comments (3)
src/cpu/pred/btb/btb_tage.cc (1)
269-274: Dead code and unused variables after simplification.After the change,
base_idxandbranch_idx(lines 270-271) are no longer used in the prediction path. The commented-out code on line 272 should also be removed.Note: The base table is still used in the update path (
updatePredictorStateAndCheckAllocation), so the table itself isn't dead—just these local computations in the prediction path.🔎 Proposed cleanup
// Generate final prediction bool main_taken = main_info.taken(); bool alt_taken = alt_info.taken(); - // Use base table instead of btb_entry.ctr - Addr base_idx = getBaseTableIndex(startPC); - unsigned branch_idx = getBranchIndexInBlock(btb_entry.pc, startPC); - //bool base_taken = getDelay() != 0 ? baseTable[base_idx][branch_idx] >= 0 : btb_entry.ctr >= 0; bool base_taken = btb_entry.ctr >= 0; bool alt_pred = alt_provided ? alt_taken : base_taken; // if alt provided, use alt prediction, otherwise use basesrc/cpu/pred/btb/mbtb.cc (2)
359-360: Consider parentheses for clarity in the taken condition.The condition
way.isIndirect||way.isDirect||way.alwaysTaken||(way.ctr >= 0)mixes different branch classification checks. While operator precedence is correct, adding parentheses improves readability.Also note: this relies on
isDirectbeing properly initialized (see related comment instream_struct.hh).🔎 Suggested improvement
- bool this_branch_taken = way.isIndirect||way.isDirect||way.alwaysTaken||(way.ctr >= 0); + bool this_branch_taken = way.isIndirect || way.isDirect || way.alwaysTaken || (way.ctr >= 0);
698-706: Remove commented-out dead code.The old update logic (lines 698-701) is commented out and should be removed for cleanliness.
🔎 Proposed cleanup
void MBTB::update(const FetchStream &stream) { DPRINTF(BTB, "BTB: update called for pc %#lx\n", stream.startPC); // 1. Check prediction hit status, for stats recording checkPredictionHit(stream, std::static_pointer_cast<BTBMeta>(stream.predMetas[getComponentIdx()]).get()); - // only update btb entry for control squash T-> NT or NT -> T - // if (stream.squashType == SQUASH_CTRL) { - // warn_if(stream.exeBranchInfo.pc > stream.updateEndInstPC, "exeBranchInfo.pc > updateEndInstPC"); - // updateBTBEntry(stream.exeBranchInfo, stream); - // } auto entries_need_update = prepareUpdateEntries(stream); for (auto &entry : entries_need_update) { updateBTBEntry(entry, stream); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/cpu/pred/btb/btb_tage.ccsrc/cpu/pred/btb/mbtb.ccsrc/cpu/pred/btb/mbtb.hhsrc/cpu/pred/btb/stream_struct.hh
🧰 Additional context used
🧬 Code graph analysis (2)
src/cpu/pred/btb/mbtb.hh (3)
src/cpu/pred/btb/btb_tage.hh (2)
stream(157-157)stream(279-279)src/cpu/pred/btb/btb_ubtb.hh (2)
stream(133-133)stream(139-139)src/cpu/pred/btb/abtb.hh (4)
stream(128-128)stream(176-176)stream(304-304)stream(310-311)
src/cpu/pred/btb/mbtb.cc (4)
src/cpu/pred/btb/mbtb.hh (12)
TickedBTBEntry(107-108)TickedBTBEntry(109-109)stream(127-127)stream(163-163)stream(171-171)stream(173-173)stream(276-277)entry(283-283)e(175-179)e(175-175)e(181-183)e(181-181)src/cpu/pred/btb/abtb.hh (8)
TickedBTBEntry(108-109)TickedBTBEntry(110-110)stream(128-128)stream(176-176)stream(304-304)stream(310-311)e(180-184)e(180-180)src/cpu/pred/btb/btb_tage.cc (2)
prepareUpdateEntries(391-417)prepareUpdateEntries(392-392)src/cpu/pred/btb/abtb.cc (2)
updateBTBEntry(488-569)updateBTBEntry(489-490)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quick Build, Unit Tests & Smoke Test
- GitHub Check: perf_test / XS-GEM5 - Run performance test (spec06-0.3c)
🔇 Additional comments (3)
src/cpu/pred/btb/stream_struct.hh (1)
101-112: LGTM!The parameterized constructor correctly initializes
isDirectfromstatic_inst->isDirectCtrl(), consistent with how other branch type flags are initialized.src/cpu/pred/btb/mbtb.hh (1)
173-174: LGTM!The new
prepareUpdateEntriesmethod declaration aligns with the similar pattern inBTBTAGEand provides a clean interface for precomputing BTB entries before updates.src/cpu/pred/btb/mbtb.cc (1)
709-731: LGTM with a note on filtering differences.The
prepareUpdateEntries()implementation correctly handles collecting and filtering BTB entries for update.Note: Unlike
BTBTAGE::prepareUpdateEntries()which filters for conditional branches (e.isCond && !e.alwaysTaken), this MBTB version only filters on resolution status. This appears intentional since MBTB handles all branch types while TAGE focuses on conditional branches.
Change-Id: If352e96f0a50c922a32f6ad41dc871cbc92a14f3
Change-Id: I49a0b5eda44fe0f5355f7177250ae83dc5b61e03
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/cpu/pred/btb/mbtb.cc (1)
544-547: Critical: Still dereferencing potentially invalid iterator.Lines 544-547 dereference
it->tickwithout first checking iffound == true. When the entry is not found in the SRAM (found == false),itequalstarget_sram[btb_idx].end(), and accessingit->tickcauses undefined behavior. This is the same issue previously flagged for line 543 in earlier reviews.🔎 Proposed fix
auto ticked_entry = TickedBTBEntry(entry_to_write, curTick()); - if (stream.exeBranchInfo.pc != ticked_entry.pc) { + if (found && stream.exeBranchInfo.pc != ticked_entry.pc) { // do not update tick if this entry is not for the executed branch ticked_entry.tick = it->tick; }Alternatively, construct with the correct tick value from the start:
- auto ticked_entry = TickedBTBEntry(entry_to_write, curTick()); - if (stream.exeBranchInfo.pc != ticked_entry.pc) { - // do not update tick if this entry is not for the executed branch - ticked_entry.tick = it->tick; - } + uint64_t initial_tick = (found && stream.exeBranchInfo.pc != entry_to_write.pc) ? it->tick : curTick(); + auto ticked_entry = TickedBTBEntry(entry_to_write, initial_tick);
🧹 Nitpick comments (2)
src/cpu/pred/btb/mbtb.cc (2)
359-359: Minor: Add spaces around operators for readability.The line has multiple
||operators without spaces, which reduces code readability.🔎 Suggested formatting improvement
- bool this_branch_taken = way.isIndirect||way.isDirect||way.alwaysTaken||(way.ctr >= 0); + bool this_branch_taken = way.isIndirect || way.isDirect || way.alwaysTaken || (way.ctr >= 0);
701-704: Remove commented-out code.The commented lines 701-704 represent the old update logic that has been replaced. If this code is no longer needed, it should be removed rather than left as comments to avoid confusion and reduce clutter.
🔎 Proposed cleanup
- // only update btb entry for control squash T-> NT or NT -> T - // if (stream.squashType == SQUASH_CTRL) { - // warn_if(stream.exeBranchInfo.pc > stream.updateEndInstPC, "exeBranchInfo.pc > updateEndInstPC"); - // updateBTBEntry(stream.exeBranchInfo, stream); - // } auto entries_need_update = prepareUpdateEntries(stream); for (auto &entry : entries_need_update) { updateBTBEntry(entry, stream); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cpu/pred/btb/mbtb.cc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quick Build, Unit Tests & Smoke Test
- GitHub Check: perf_test / XS-GEM5 - Run performance test (spec06-0.3c)
🔇 Additional comments (2)
src/cpu/pred/btb/mbtb.cc (2)
727-731: Critical: Filter condition appears inverted.Line 729 checks
!e.resolvedto filter entries, but ifgetResolvedUpdate()returnstrue, the intent is likely to keep only resolved entries (those withe.resolved == true), not non-resolved ones. The current logic keeps non-resolved entries, which appears to be the opposite of the intended behavior.🔎 Proposed fix
// Filter: only keep conditional branches that are not always taken if (getResolvedUpdate()) { auto remove_it = std::remove_if(all_entries.begin(), all_entries.end(), - [](const BTBEntry &e) { return !( e.resolved); }); + [](const BTBEntry &e) { return !e.resolved; }); all_entries.erase(remove_it, all_entries.end()); }This removes the extra parentheses and negation, keeping only entries where
e.resolved == truewhengetResolvedUpdate()is true.Likely an incorrect or invalid review comment.
558-560: Verify this insertion condition aligns with PR objectives.The code conditionally inserts new BTB entries only when both
stream.exeTakenis true AND the entry matches the executed branch PC. This means entries for branches that are not taken will not be inserted into the BTB—a deliberate behavior change that reduces BTB coverage for non-taken branches.Confirm this restriction is intentional and documented as part of the replacement algorithm changes.
Change-Id: I2bc707fefe6648874a3c12190e669bd0933c9e7f
Change-Id: Iebed52eb9e3a60ac5ef3bbc0e96095d2d16ef0fa
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/cpu/pred/btb/mbtb.cc (1)
544-547: Critical: Dereferencing potentially invalid iterator.Line 546 accesses
it->tickwithout checking iffound == true. When the entry is not found in the SRAM (line 519),itequalstarget_sram[btb_idx].end(), and dereferencing it causes undefined behavior. This affects the!found && !found_in_vcpath where a new entry is being inserted.🔎 Recommended fix
auto entry_to_write = buildUpdatedEntry(entry, existing_ptr, stream, btb_tag); - auto ticked_entry = TickedBTBEntry(entry_to_write, curTick()); - if (stream.exeBranchInfo.pc != ticked_entry.pc) { - // do not update tick if this entry is not for the executed branch - ticked_entry.tick = it->tick; - } + uint64_t initial_tick = (found && stream.exeBranchInfo.pc != entry_to_write.pc) ? it->tick : curTick(); + auto ticked_entry = TickedBTBEntry(entry_to_write, initial_tick);Alternatively, move the tick adjustment inside the
if (found)block after line 549.
🧹 Nitpick comments (3)
src/cpu/pred/btb/mbtb.cc (3)
359-360: Consider improving operator spacing for readability.The conditional MRU update logic is correct, but line 359 would be more readable with spaces around operators.
🔎 Suggested readability improvement
- bool this_branch_taken = way.isIndirect||way.isDirect||way.alwaysTaken||(way.ctr >= 0); + bool this_branch_taken = way.isIndirect || way.isDirect || way.alwaysTaken || (way.ctr >= 0);
701-708: Remove commented-out code.The new update logic using
prepareUpdateEntriesis correct and more flexible than the previous single-entry update. However, the commented-out code on lines 701-704 should be removed to maintain code cleanliness.🔎 Proposed cleanup
checkPredictionHit(stream, std::static_pointer_cast<BTBMeta>(stream.predMetas[getComponentIdx()]).get()); - // only update btb entry for control squash T-> NT or NT -> T - // if (stream.squashType == SQUASH_CTRL) { - // warn_if(stream.exeBranchInfo.pc > stream.updateEndInstPC, "exeBranchInfo.pc > updateEndInstPC"); - // updateBTBEntry(stream.exeBranchInfo, stream); - // } auto entries_need_update = prepareUpdateEntries(stream); for (auto &entry : entries_need_update) { updateBTBEntry(entry, stream); }
712-734: Consider adding function documentation.The implementation correctly builds a dynamic list of entries to update, handling both existing entries and potential new entries. The resolved-flag filtering aligns well with the test changes in btb.test.cc. However, adding a function-level comment explaining the filtering logic and return value would improve maintainability.
Example documentation
/** * Prepare the list of BTB entries that need to be updated. * * @param stream The FetchStream containing execution results * @return Vector of BTBEntry objects to update * * This function: * 1. Starts with entries from stream.updateBTBEntries * 2. Adds potential new entry if it was a BTB miss during prediction * 3. Filters out unresolved entries if getResolvedUpdate() returns true */ std::vector<BTBEntry> MBTB::prepareUpdateEntries(const FetchStream &stream) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cpu/pred/btb/mbtb.ccsrc/cpu/pred/btb/test/btb.test.cc
🧰 Additional context used
🧬 Code graph analysis (1)
src/cpu/pred/btb/test/btb.test.cc (2)
src/cpu/pred/btb/mbtb.hh (6)
entry(283-283)stream(127-127)stream(163-163)stream(171-171)stream(173-173)stream(276-277)src/cpu/pred/btb/stream_struct.hh (1)
entry(494-516)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quick Build, Unit Tests & Smoke Test
- GitHub Check: perf_test / XS-GEM5 - Run performance test (spec06-0.3c)
🔇 Additional comments (2)
src/cpu/pred/btb/test/btb.test.cc (1)
135-137: LGTM! Proper test setup for resolved entries.Marking all entries as resolved before calling
update()ensures they pass the filtering inprepareUpdateEntries(lines 727-731 in mbtb.cc) whengetResolvedUpdate()returns true. This is appropriate test setup.src/cpu/pred/btb/mbtb.cc (1)
558-560: LGTM! Appropriate guard for selective replacement.The condition ensures that only the executed taken branch triggers a replacement when the entry is not found in SRAM or victim cache. This aligns with the new selective update strategy introduced by
prepareUpdateEntries.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/cpu/pred/btb/mbtb.cc (1)
545-548: Critical: Dereferencing potentially invalid iterator.When
found == false, the iteratoritequalstarget_sram[btb_idx].end(), andit->tickon line 547 causes undefined behavior. This only executes when the executed branch PC differs from the entry PC, but the bug exists whenfoundis false.🔎 Proposed fix
auto entry_to_write = buildUpdatedEntry(entry, existing_ptr, stream, btb_tag); auto ticked_entry = TickedBTBEntry(entry_to_write, curTick()); if (stream.exeBranchInfo.pc != ticked_entry.pc) { - // do not update tick if this entry is not for the executed branch - ticked_entry.tick = it->tick; + // do not update tick if this entry is not for the executed branch + if (found) { + ticked_entry.tick = it->tick; + } + // else: keep curTick() for new entries }
🧹 Nitpick comments (2)
src/cpu/pred/btb/mbtb.hh (1)
398-398: Consider grouping configuration flags together.The
usingBasetablemember is placed between address calculation fields (tagBitsandtagMask). Consider relocating it near other configuration-related members for better code organization.src/cpu/pred/btb/btb_tage.cc (1)
273-276: Consider refactoring the nested ternary for clarity.The nested ternary expression is functionally correct but hard to parse at a glance. A brief inline comment or restructuring could improve maintainability.
🔎 Suggested refactor
- bool base_taken = getDelay() != 0 ? (usingBasetable ? baseTable[base_idx][branch_idx] >= 0 : btb_entry.ctr >= 0) - : btb_entry.ctr >= 0; - //bool base_taken = btb_entry.ctr >= 0; + // Base prediction source: use baseTable when TAGE owns the basetable and has delay, + // otherwise fall back to BTB entry counter. + bool base_taken; + if (getDelay() != 0 && usingBasetable) { + base_taken = baseTable[base_idx][branch_idx] >= 0; + } else { + base_taken = btb_entry.ctr >= 0; + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
configs/example/kmhv3.pysrc/cpu/pred/BranchPredictor.pysrc/cpu/pred/btb/btb_tage.ccsrc/cpu/pred/btb/btb_tage.hhsrc/cpu/pred/btb/mbtb.ccsrc/cpu/pred/btb/mbtb.hh
🧰 Additional context used
🧬 Code graph analysis (3)
configs/example/kmhv3.py (1)
src/cpu/pred/btb/test/btb_tage.test.cc (1)
tage(276-282)
src/cpu/pred/btb/mbtb.hh (1)
src/cpu/pred/btb/btb_tage.hh (3)
stream(157-157)stream(279-279)stream(421-421)
src/cpu/pred/btb/mbtb.cc (2)
src/cpu/pred/btb/abtb.cc (2)
updateBTBEntry(488-569)updateBTBEntry(489-490)src/cpu/pred/btb/btb_tage.cc (2)
prepareUpdateEntries(393-419)prepareUpdateEntries(394-394)
🪛 Ruff (0.14.10)
src/cpu/pred/BranchPredictor.py
988-988: Param may be undefined, or defined from star imports
(F405)
1064-1064: Param may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quick Build, Unit Tests & Smoke Test
- GitHub Check: perf_test / XS-GEM5 - Run performance test (spec06-0.3c)
🔇 Additional comments (11)
src/cpu/pred/btb/btb_tage.hh (1)
318-318: LGTM!The new
usingBasetableflag is appropriately placed in the private section and provides the necessary gating for switching between basetable-based and CTR-based base predictions.src/cpu/pred/btb/mbtb.hh (1)
173-174: LGTM!The new
prepareUpdateEntriesmethod declaration aligns with the corresponding implementation and mirrors the same method inBTBTAGE, enabling consistent update path behavior across both components.configs/example/kmhv3.py (1)
111-114: LGTM!The configuration wiring correctly propagates the
usingMbtbBaseEiterTageflag to both MBTB and TAGE components, ensuring consistent behavior. Using a local variable to set both is a clean approach.src/cpu/pred/btb/btb_tage.cc (1)
94-94: LGTM!The negated initialization
usingBasetable( !p.usingMbtbBaseEiterTage)correctly implements the logic: when MBTB's basetable is used (usingMbtbBaseEiterTage=True), BTBTAGE should fall back tobtb_entry.ctrinstead of its own basetable.src/cpu/pred/BranchPredictor.py (2)
988-988: LGTM!The new
usingMbtbBaseEiterTageparameter correctly exposes the basetable usage configuration for MBTB. The Ruff F405 warning is a false positive—Paramis properly imported via star import fromm5.params, which is standard practice in gem5 configuration files.
1064-1064: LGTM!The parameter is consistently added to BTBTAGE with the same default value as MBTB, ensuring coherent configuration across both components.
src/cpu/pred/btb/mbtb.cc (5)
84-84: LGTM!The constructor correctly initializes
usingBasetablefrom the configuration parameter without negation, unlike BTBTAGE which inverts the flag. This is intentional—MBTB uses its basetable when the flag is true.
360-361: LGTM!The MRU timestamp is now only updated when the branch is predicted taken, improving cache replacement behavior by not refreshing timestamps for not-taken predictions.
559-561: Verify intentional restriction of new entry insertion.The new condition restricts SRAM replacement to only when
stream.exeTaken && stream.exeBranchInfo.pc == ticked_entry.pc. This means entries for non-executed or not-taken branches won't be inserted. Confirm this is the intended behavior, as it differs from how updates worked previously.
700-712: LGTM!The update path correctly branches based on
usingBasetable: when disabled, only control squash events trigger updates; when enabled, the dynamic entry list fromprepareUpdateEntriesis processed. The logic is clean and aligns with the design intent.
716-738: Verify filtering difference from BTBTAGE's prepareUpdateEntries.MBTB's
prepareUpdateEntriesfilters only one.resolvedwhengetResolvedUpdate()is true, keeping all resolved entries. BTBTAGE filters one.isCond && !e.alwaysTaken && e.resolved(whengetResolvedUpdate()is true) ore.isCond && !e.alwaysTaken(when false), keeping only conditional non-always-taken branches. This difference means MBTB processes resolved unconditional and always-taken branches, while BTBTAGE skips them. Confirm this behavioral difference is intentional.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
|
[Generated by GEM5 Performance Robot] Standard PerformanceOverall Score
[Generated by GEM5 Performance Robot] Standard PerformanceOverall Score
|
Change-Id: I310df13c9d1dfeac320c51940c4de486a954ab06
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cpu/pred/btb/mbtb.cc (1)
709-731: Consider extracting the duplicatedprepareUpdateEntrieslogic.This method is identical to
BTBTAGE::prepareUpdateEntries(seesrc/cpu/pred/btb/btb_tage.cclines 392-418). Both implementations:
- Copy
stream.updateBTBEntries- Conditionally add
stream.updateNewBTBEntrywithalwaysTakenadjustment- Filter by
resolvedstatus whengetResolvedUpdate()is trueRefactoring this shared logic into a common base class or utility function would reduce maintenance burden and ensure consistent behavior across both components.
💡 Refactoring approach
Consider moving this method to a shared base class (e.g.,
TimedBaseBTBPredictor) or a utility namespace, since both MBTB and BTBTAGE require identical logic for preparing update entries whenusingBasetableis enabled.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cpu/pred/btb/mbtb.cc
🧰 Additional context used
🧬 Code graph analysis (1)
src/cpu/pred/btb/mbtb.cc (1)
src/cpu/pred/btb/btb_tage.cc (2)
prepareUpdateEntries(393-419)prepareUpdateEntries(394-394)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: perf_test / XS-GEM5 - Run performance test (spec06-0.3c)
🔇 Additional comments (3)
src/cpu/pred/btb/mbtb.cc (3)
84-84: Initialization ofusingBasetablelooks correct.The parameter is properly initialized from
p.usingMbtbBaseEiterTageand aligns with the new basetable-based configuration introduced in this PR.
693-706: Verify the behavior change whenusingBasetable == true.The update logic has changed significantly:
- Legacy path (
!usingBasetable): Updates only occur whensquashType == SQUASH_CTRL(control transitions T→NT or NT→T).- New path (
usingBasetable): Updates occur for all entries fromprepareUpdateEntries(), regardless of squash type.This means the new path updates BTB entries even when there's no control squash. Confirm that this broader update policy aligns with the intended basetable-based replacement strategy described in the PR objectives.
534-543: Past iterator dereference issue appears resolved.The previous critical issue flagged in past reviews (unconditional dereference of
it->tickwhenitcould beend()) has been addressed. The code now:
- Sets
existing_ptrbased on whether an entry was found (lines 534-540)- Uses
curTick()unconditionally when constructingTickedBTBEntry(line 543)This approach safely handles all cases (
found,found_in_vc, or neither) without dereferencing invalid iterators.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
3 participants
move basetable from tage to mbtb and change mbtb entry Replacement algorithm
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.