Conversation
Change-Id: Id1b9d9e656ce068f27a04ec885344c5f325019d6
Change-Id: Id8ab7df0c0755c1a6d0b3069de9fa3f62c6a9687
Change-Id: I529d8a3c3ad50b95e5af19c0b16132c1173adabc
Change-Id: I375511ae4f9d7238e4baba61ed62bc55a7400526
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworks BTB TAGE allocation and usefulness semantics: converts the useful flag to a small counter with helper APIs, aligns allocation priority to invalid → weak-not-useful → any-not-useful, consolidates allocation flow and stats, updates microTAGE allocation and tests, and adds allocation-related telemetry and docs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d3ba69ccd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/cpu/pred/btb/btb_tage.cc
Outdated
| usefulResetCnt++; | ||
| } | ||
|
|
||
| usefulResetCnt++; |
There was a problem hiding this comment.
Incrementing usefulResetCnt unconditionally here means mispredictions whose provider is already in the longest-history table (start_table == numPredictors) still advance the global reset counter even though no allocation set is probed. After enough such events, the code resets usefulness for all entries spuriously, which can destabilize trained entries in unrelated tables and skew long-running performance results. The counter should only advance when at least one candidate table was examined and no victim was found.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cpu/pred/btb/microtage.cc (2)
551-556: Potential asymmetry in usefulResetCnt update.On successful allocation (line 551), the counter is decremented:
usefulResetCnt = usefulResetCnt <= 0 ? 0 : usefulResetCnt - 1;On allocation failure (line 556), the counter is incremented:
usefulResetCnt++;However, in
btb_tage.cc(line 622), the counter is only incremented after all tables have been tried (outside the loop), and there's no decrement on success.This behavioral difference means microtage will reset useful bits less frequently than btb_tage under similar allocation patterns. Please verify this asymmetry is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/pred/btb/microtage.cc` around lines 551 - 556, The update of usefulResetCnt is asymmetric: in microtage.cc you decrement usefulResetCnt on a successful allocation and increment it immediately on any allocation failure (usefulResetCnt = usefulResetCnt <= 0 ? 0 : usefulResetCnt - 1; and usefulResetCnt++ where tageStats.updateAllocFailure++), whereas btb_tage.cc only increments that counter after all tables have been tried; decide which policy is correct and make them consistent — either move the increment in microtage.cc to the same post-loop spot (so usefulResetCnt++ occurs only after all tables fail) or add a corresponding decrement in btb_tage.cc on success; update the code paths around usefulResetCnt, the allocation success branch and the tageStats.updateAllocFailure increment so both implementations follow the same semantics.
564-568: Type inconsistency: usingfalsefor useful reset.Line 566 assigns
way.useful = false, which works because microtage'sTageEntry.usefulis stillbool. However, this differs frombtb_tage.ccwhich usesusefulCtrReset(way)to setuseful = usefulCtrInit(uint8_t 0).If microtage is intended to eventually migrate to the 2-bit counter semantics like btb_tage, consider:
- Using
way.useful = 0instead ofway.useful = falsefor forward compatibility- Or keeping as-is if microtage intentionally uses different useful semantics
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/pred/btb/microtage.cc` around lines 564 - 568, The loop currently resets useful with a boolean (way.useful = false) which is inconsistent with btb_tage's 2-bit counter semantics; change the reset to use the same representation by assigning the integer zero (way.useful = 0) or call the existing helper usefulCtrReset(way) so useful is initialized to usefulCtrInit (uint8_t 0) — update the loop that iterates over table and way to use way.useful = 0 or usefulCtrReset(way) to match btb_tage semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cpu/pred/btb/microtage.cc`:
- Around line 551-556: The update of usefulResetCnt is asymmetric: in
microtage.cc you decrement usefulResetCnt on a successful allocation and
increment it immediately on any allocation failure (usefulResetCnt =
usefulResetCnt <= 0 ? 0 : usefulResetCnt - 1; and usefulResetCnt++ where
tageStats.updateAllocFailure++), whereas btb_tage.cc only increments that
counter after all tables have been tried; decide which policy is correct and
make them consistent — either move the increment in microtage.cc to the same
post-loop spot (so usefulResetCnt++ occurs only after all tables fail) or add a
corresponding decrement in btb_tage.cc on success; update the code paths around
usefulResetCnt, the allocation success branch and the
tageStats.updateAllocFailure increment so both implementations follow the same
semantics.
- Around line 564-568: The loop currently resets useful with a boolean
(way.useful = false) which is inconsistent with btb_tage's 2-bit counter
semantics; change the reset to use the same representation by assigning the
integer zero (way.useful = 0) or call the existing helper usefulCtrReset(way) so
useful is initialized to usefulCtrInit (uint8_t 0) — update the loop that
iterates over table and way to use way.useful = 0 or usefulCtrReset(way) to
match btb_tage semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97e25ad5-5748-42c3-8016-7003ff53b07c
📒 Files selected for processing (4)
src/cpu/pred/btb/btb_tage.ccsrc/cpu/pred/btb/btb_tage.hhsrc/cpu/pred/btb/microtage.ccsrc/cpu/pred/btb/test/btb_tage.test.cc
Change-Id: I2f26d847bf2978d83b7a3a1cc63ee887184e4fd2
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cpu/pred/btb/btb_tage.cc (1)
573-624: Consider the allocation probe counting logic.The
allocProbeNoEligibleVictimstat is incremented inside the loop (line 624) for each table that has no eligible victim, but then also used to track "no allocation found" scenarios. However, when a successful allocation occurs, the function returns early (line 621), so the counter tracks per-table failures correctly.One observation: when all ways in a table have
useful > 0, the statallocBucketAllUsefulOrNoCandidateis incremented (line 609), but thenallocProbeNoEligibleVictimis also incremented (line 624). This double-counting might make the stats harder to interpret.📊 Consider restructuring stats for clearer semantics
The current flow increments both
allocBucketAllUsefulOrNoCandidateandallocProbeNoEligibleVictimwhen no victim is found. If these are meant to be mutually exclusive categories, consider:} else { tageStats.allocBucketAllUsefulOrNoCandidate++; } if (selected_way != -1) { // ... allocation success ... return true; } - - tageStats.allocProbeNoEligibleVictim++; } + + // Only count probes with no eligible victim after exhausting all tables + tageStats.allocProbeNoEligibleVictim++;However, if the intent is to count per-table failures, the current logic is correct. A comment clarifying the semantic would help.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/pred/btb/btb_tage.cc` around lines 573 - 624, The per-table path increments both tageStats.allocBucketAllUsefulOrNoCandidate and then later tageStats.allocProbeNoEligibleVictim, causing double-counting when a table has no eligible victim; change the logic so these are mutually exclusive by removing the per-table increment of allocProbeNoEligibleVictim and only incrementing allocProbeNoEligibleVictim once when the overall probe fails (i.e., after the loop when selected_way == -1 and no allocation occurred), or alternatively stop incrementing allocBucketAllUsefulOrNoCandidate and keep allocProbeNoEligibleVictim per-table—update the code around the selected_way decision (references: selected_way, allocBucketAllUsefulOrNoCandidate, allocProbeNoEligibleVictim, allocated_table/allocated_index and the early return true) to ensure only one counter is incremented for a table with no victim.src/cpu/pred/btb/test/btb_tage.test.cc (1)
440-450: Verify the expected allocation behavior when provider is at highest table.The test expects both
updateAllocFailureandupdateAllocSuccessto be 0 when the provider is at the highest table. This aligns with the RTL-gating logic at lines 523-526 of btb_tage.cc that suppresses allocation. However, the comment and assertion messages could be slightly clearer.📝 Consider clarifying the test comment
- // Update the predictor. With RTL-aligned highest-table gating, this should - // not even start allocation when the provider already comes from the - // highest table. + // Update the predictor. With RTL-aligned highest-table gating, allocation + // is completely suppressed when the provider already comes from the + // highest table (no allocation attempt is made, hence no success or failure).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/pred/btb/test/btb_tage.test.cc` around lines 440 - 450, The test verifies that calling tage->update(stream) when the provider is from the highest table does not trigger allocation; update the inline comment and the EXPECT_EQ assertion messages to be more explicit about why both tage->tageStats.updateAllocFailure and tage->tageStats.updateAllocSuccess should remain zero (e.g., mention "RTL-aligned highest-table gating suppresses any allocation attempt or failure when provider originates from highest table"), and ensure the comment sits immediately above the tage->update(stream) call so readers can easily correlate the behavior with the expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cpu/pred/btb/btb_tage.cc`:
- Around line 573-624: The per-table path increments both
tageStats.allocBucketAllUsefulOrNoCandidate and then later
tageStats.allocProbeNoEligibleVictim, causing double-counting when a table has
no eligible victim; change the logic so these are mutually exclusive by removing
the per-table increment of allocProbeNoEligibleVictim and only incrementing
allocProbeNoEligibleVictim once when the overall probe fails (i.e., after the
loop when selected_way == -1 and no allocation occurred), or alternatively stop
incrementing allocBucketAllUsefulOrNoCandidate and keep
allocProbeNoEligibleVictim per-table—update the code around the selected_way
decision (references: selected_way, allocBucketAllUsefulOrNoCandidate,
allocProbeNoEligibleVictim, allocated_table/allocated_index and the early return
true) to ensure only one counter is incremented for a table with no victim.
In `@src/cpu/pred/btb/test/btb_tage.test.cc`:
- Around line 440-450: The test verifies that calling tage->update(stream) when
the provider is from the highest table does not trigger allocation; update the
inline comment and the EXPECT_EQ assertion messages to be more explicit about
why both tage->tageStats.updateAllocFailure and
tage->tageStats.updateAllocSuccess should remain zero (e.g., mention
"RTL-aligned highest-table gating suppresses any allocation attempt or failure
when provider originates from highest table"), and ensure the comment sits
immediately above the tage->update(stream) call so readers can easily correlate
the behavior with the expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e9a0ae3-3c03-4787-965d-f699b0484fe8
📒 Files selected for processing (3)
src/cpu/pred/btb/btb_tage.ccsrc/cpu/pred/btb/btb_tage.hhsrc/cpu/pred/btb/test/btb_tage.test.cc
Change-Id: I5c28f6c692b1b1e0d8c8581952353ac6974ab1ca
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cpu/pred/btb/test/btb_tage.test.cc (1)
735-736: Minor style inconsistency withusefulparameter.The
createManualTageEntryfunction takes abool usefulparameter (line 699), but callers now pass0instead offalse. While this works due to implicit conversion, usingfalseexplicitly would be clearer and more consistent with the parameter type.♻️ Suggested consistency fix
- createManualTageEntry(tage, testTable, testIndex, 0, testTag1, 2, 0, 0x1000, 0); // Way 0: Strong taken - createManualTageEntry(tage, testTable, testIndex, 1, testTag2, -2, 0, 0x1004, 1); // Way 1: Strong not taken + createManualTageEntry(tage, testTable, testIndex, 0, testTag1, 2, false, 0x1000, 0); // Way 0: Strong taken + createManualTageEntry(tage, testTable, testIndex, 1, testTag2, -2, false, 0x1004, 1); // Way 1: Strong not taken🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/pred/btb/test/btb_tage.test.cc` around lines 735 - 736, The call sites to createManualTageEntry (e.g., the two calls shown) pass the `useful` argument as integer literals `0`; change those to the boolean literal `false` to match the function signature (createManualTageEntry(..., bool useful)) and improve readability/consistency—update any other callers that use `0`/`1` for the `useful` parameter to `false`/`true` respectively.src/cpu/pred/btb/btb_tage.cc (1)
573-596: Consider skipping invalid entries in the second loop.The second loop (lines 582-596) searches for weak-not-useful and strong-not-useful candidates, but it doesn't skip entries where
!set[way].valid. If an invalid entry hasuseful=falseby default, it could be added toweak_not_useful_wayorstrong_not_useful_wayredundantly.While the priority logic at lines 599-610 correctly prefers
invalid_wayfirst, adding an earlycontinuefor invalid entries in the second loop would improve clarity and avoid redundant checks:♻️ Suggested refinement
for (unsigned way = 0; way < numWays; ++way) { auto &cand = set[way]; + if (!cand.valid) { + continue; + } if (!usefulCtrIsSaturateNegative(cand)) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/pred/btb/btb_tage.cc` around lines 573 - 596, The second loop that sets weak_not_useful_way and strong_not_useful_way should skip invalid entries to avoid selecting an already-invalid way; inside the loop iterating over set[way] (the same loop that uses usefulCtrIsSaturateNegative and computes weakish from cand.counter) add an early check for set[way].valid (or !set[way].valid) and continue if the entry is invalid so only valid candidates are considered when setting weak_not_useful_way and strong_not_useful_way.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cpu/pred/btb/btb_tage.cc`:
- Around line 573-596: The second loop that sets weak_not_useful_way and
strong_not_useful_way should skip invalid entries to avoid selecting an
already-invalid way; inside the loop iterating over set[way] (the same loop that
uses usefulCtrIsSaturateNegative and computes weakish from cand.counter) add an
early check for set[way].valid (or !set[way].valid) and continue if the entry is
invalid so only valid candidates are considered when setting weak_not_useful_way
and strong_not_useful_way.
In `@src/cpu/pred/btb/test/btb_tage.test.cc`:
- Around line 735-736: The call sites to createManualTageEntry (e.g., the two
calls shown) pass the `useful` argument as integer literals `0`; change those to
the boolean literal `false` to match the function signature
(createManualTageEntry(..., bool useful)) and improve
readability/consistency—update any other callers that use `0`/`1` for the
`useful` parameter to `false`/`true` respectively.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dd7c2e0-313b-4df6-8064-a1b691219b48
📒 Files selected for processing (4)
.codex/skills/ci-perf-analysis/SKILL.mdsrc/cpu/pred/btb/btb_tage.ccsrc/cpu/pred/btb/btb_tage.hhsrc/cpu/pred/btb/test/btb_tage.test.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cpu/pred/btb/btb_tage.hh
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
Change-Id: Ic4ebba4d7249d1cdbdd2b6a8121e3b400a41d75a
Change-Id: I66180a45246dad557df552c7d4c731ec9e1c2dc3
Change-Id: I31fe4c0da0cdaa4eb3cfba8ebd83a84e7db72d30
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cpu/pred/btb/test/btb_tage.test.cc (1)
213-224: Consider updatingusefulparameter type for clarity.The
setupTageEntryhelper still usesbool useful(line 214), but the underlyingTageEntry::usefulfield appears to have been changed to a counter type based on the implementation changes. Whileboolassignment works (converting to 0/1), usinguint8_tor the actual counter type would be clearer and prevent potential confusion.The same applies to
createManualTageEntryat line 745.🔧 Optional: Update parameter type for consistency
void setupTageEntry(BTBTAGE* tage, Addr pc, int table_idx, - short counter, bool useful = false, int way = 0) { + short counter, uint8_t useful = 0, int way = 0) {void createManualTageEntry(BTBTAGE* tage, int table, Addr index, int way, - Addr tag, short counter, bool useful, Addr pc, + Addr tag, short counter, uint8_t useful, Addr pc, unsigned lruCounter = 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/pred/btb/test/btb_tage.test.cc` around lines 213 - 224, The helper setupTageEntry uses a bool for the useful parameter but the underlying field TageEntry::useful is a small counter type; change the parameter type from bool useful to the actual counter type (e.g., uint8_t usefulCounter or the specific typedef used by TageEntry) and update the assignment entry.useful = usefulCounter so the conversion is explicit; do the same change for createManualTageEntry (update its parameter name and type to match TageEntry::useful's type) to keep signatures consistent and avoid implicit bool-to-integer conversions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cpu/pred/btb/test/btb_tage.test.cc`:
- Around line 213-224: The helper setupTageEntry uses a bool for the useful
parameter but the underlying field TageEntry::useful is a small counter type;
change the parameter type from bool useful to the actual counter type (e.g.,
uint8_t usefulCounter or the specific typedef used by TageEntry) and update the
assignment entry.useful = usefulCounter so the conversion is explicit; do the
same change for createManualTageEntry (update its parameter name and type to
match TageEntry::useful's type) to keep signatures consistent and avoid implicit
bool-to-integer conversions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb529c74-4212-47cf-8d68-41681848bcce
📒 Files selected for processing (4)
.codex/skills/rtl-tage-alignment/SKILL.mddocs/progress/TAGE-align.mdsrc/cpu/pred/btb/btb_tage.ccsrc/cpu/pred/btb/test/btb_tage.test.cc
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
1 similar comment
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
Change-Id: I72bf16c22105756a6cdd71bfbee2f1b203dcebd8
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
1 similar comment
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
Change-Id: I9f7d6f509712b0e8d42542725d7541df1669c13f
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
2 participants
Summary by CodeRabbit
Improvements
Tests
Statistics
Documentation