Conversation
|
🚀 Performance test triggered: spec06-0.8c |
📝 WalkthroughWalkthroughRenames and removes FSQ/FetchStream identifiers in favor of FTQ/FetchTarget across O3 pipeline, fetch/BTB predictors, and related tests; removes squashedStreamId and consolidates stream bookkeeping to FTQ-based fields without changing higher-level behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Fetch as FetchUnit
participant BTB as DecoupledBPU/BTB
participant Decode as Decode
participant IEW as IEW
participant Commit as Commit
Fetch->>BTB: create FetchTarget(ftqId) / enqueue prediction
BTB-->>Fetch: prediction (FetchTarget carries ftqId)
Fetch->>Decode: build instruction with ftqId
Decode->>IEW: forward dynInst(ftqId)
IEW->>Commit: commit signal doneFtqId / doneFtqId used for updates
Commit->>BTB: update/notify using ftqId (doneFtqId)
BTB->>BTB: prepareResolveUpdateEntries(mark CFI resolved using ftqId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/cpu/pred/btb/test/btb.test.cc (1)
48-67: Refresh docstring to match FetchTarget.The helper’s docs still refer to “FetchStream” even though the type is now
FetchTarget. Updating these comments will avoid confusion.📝 Suggested doc fix
- * `@brief` Setup a FetchStream with common parameters for BTB update + * `@brief` Setup a FetchTarget with common parameters for BTB update @@ - * `@return` FetchStream Initialized fetch stream + * `@return` FetchTarget Initialized fetch targetsrc/cpu/pred/btb/common.hh (1)
276-354: Missing initialization oftidmember in constructor.The new
tidmember added at line 278 is not initialized in theFetchTargetconstructor. SinceThreadIDis typically an integer type, this will leavetidwith an indeterminate value, which could cause subtle bugs when the field is later read.Proposed fix
FetchTarget() - : startPC(0), + : tid(0), + startPC(0), predTaken(false),src/cpu/pred/btb/btb_tage.hh (1)
146-150: Type mismatch in UNIT_TEST compatibility wrapper.The
recoverHistwrapper underUNIT_TESTstill usesconst FetchStream &entryparameter, but it forwards torecoverPHistwhich now expectsconst FetchTarget &entry. IfFetchStreamandFetchTargetare distinct types, this will cause a compilation error in unit test builds.Suggested fix
- void recoverHist(const boost::dynamic_bitset<> &history, const FetchStream &entry, int shamt, + void recoverHist(const boost::dynamic_bitset<> &history, const FetchTarget &entry, int shamt, bool cond_taken) override { recoverPHist(history, entry, shamt, cond_taken); }src/cpu/pred/btb/btb_mgsc.cc (1)
540-557: Filter predicate doesn’t match the comment intent.Line 546 only removes entries that are both non-conditional and not always-taken, so always-taken entries still pass. If the intent is “keep only conditional and not always-taken,” this should be
!e.isCond || e.alwaysTaken(or update the comment if the current behavior is intended).🔧 Suggested fix
- auto remove_it = std::remove_if(all_entries.begin(), all_entries.end(), - [](const BTBEntry &e) { return !e.isCond && !e.alwaysTaken; }); + auto remove_it = std::remove_if(all_entries.begin(), all_entries.end(), + [](const BTBEntry &e) { return !e.isCond || e.alwaysTaken; });src/cpu/o3/fetch.cc (1)
1512-1555: Update resolve APIs to accept full 64-bit FetchTargetId instead of narrowing to 32-bit.Line 1544 narrows
entry.resolvedFTQId(uint64_t) tounsigned int, truncating the FTQ ID and causing incorrect queue lookups and predictor updates. The root cause is the resolve API signatures—resolveUpdate,prepareResolveUpdateEntries, andmarkCFIResolved—which acceptunsigned &target_idinstead ofFetchTargetId. Change both the API signatures insrc/cpu/pred/btb/decoupled_bpred.hhand their implementations, and useFetchTargetId stream_idat line 1544.Required changes
decoupled_bpred.hh (update signatures):
- bool resolveUpdate(unsigned &target_id); - void prepareResolveUpdateEntries(unsigned &target_id); - void markCFIResolved(unsigned &target, uint64_t resolvedInstPC); + bool resolveUpdate(FetchTargetId target_id); + void prepareResolveUpdateEntries(FetchTargetId target_id); + void markCFIResolved(FetchTargetId target, uint64_t resolvedInstPC);fetch.cc (line 1544):
- unsigned int stream_id = entry.resolvedFTQId; + FetchTargetId stream_id = entry.resolvedFTQId;src/cpu/pred/btb/decoupled_bpred.cc (1)
890-898: Variable naming inconsistency.The variable
fsqidon line 894 should be renamed toftqid(orftq_id) to align with the FTQ terminology used throughout the rest of the file.🔧 Suggested fix
Addr DecoupledBPUWithBTB::getPreservedReturnAddr(const DynInstPtr &dynInst) { DPRINTF(DecoupleBP, "acquiring reutrn address for inst pc %#lx from decode\n", dynInst->pcState().instAddr()); - auto fsqid = dynInst->getFtqId(); - auto retAddr = ras->getTopAddrFromMetas(getTarget(fsqid)); + auto ftqid = dynInst->getFtqId(); + auto retAddr = ras->getTopAddrFromMetas(getTarget(ftqid)); DPRINTF(DecoupleBP, "get ret addr %#lx\n", retAddr); return retAddr; }
🤖 Fix all issues with AI agents
In `@src/cpu/o3/commit.cc`:
- Around line 1321-1323: The current branch only updates
toIEW->commitInfo[tid].doneFtqId when head_inst->getFtqId() > 1, leaving a stale
value when ftqId is 1 or 0; add an else branch that explicitly resets
toIEW->commitInfo[tid].doneFtqId (e.g., set to 0 or another sentinel
representing "none") when head_inst->getFtqId() <= 1 so downstream FTQ
bookkeeping cannot read a stale value; locate this around the
head_inst->getFtqId() check in commit.cc and update the else-path to clear the
doneFtqId for the given tid.
In `@src/cpu/o3/dyn_inst.hh`:
- Around line 398-402: Update the stale comments for the fields ftqId and
loopIteration to use FTQ terminology instead of FSQ/fetch stream queue: change
the comment above ftqId to say it is the FTQ (fetch target queue) ID used for
squashing and committing, and update the loopIteration comment to reference the
loop iteration within an FTQ entry (or FTQ entry metadata) rather than an "fsq
entry"; ensure the comments around the unsigned ftqId and unsigned loopIteration
match naming of FTQ used elsewhere in the codebase.
In `@src/cpu/pred/btb/abtb.cc`:
- Line 33: Replace the inconsistent relative include in abtb.cc—currently
including "common.hh"—with the same full include path used elsewhere
("cpu/pred/btb/common.hh") so it matches the header abtb.hh and other files;
update the include directive in abtb.cc to reference cpu/pred/btb/common.hh to
prevent build failures due to differing include search configurations.
In `@src/cpu/pred/btb/btb_ubtb.cc`:
- Line 34: Replace the relative include in btb_ubtb.cc: change the `#include`
"common.hh" to the full module path "#include \"cpu/pred/btb/common.hh\"" so it
matches btb_ubtb.hh and the rest of the module; this keeps include style
consistent across the BTB files and avoids mismatched headers.
In `@src/cpu/pred/btb/decoupled_bpred.cc`:
- Around line 402-405: The debug message in the DPRINTF call for DecoupleBP uses
outdated variable names in the format string ("fsqId" and "fetchHeadFsqId")
while the actual variables are ftqId and fetchHeadFtqId; update the format
string passed to DPRINTF(DecoupleBP, ...) to reference ftqId and fetchHeadFtqId
(keep the s0PC reference and existing format specifiers %#lx/%lu as appropriate)
so the printed labels match the variables ftqId, fetchHeadFtqId, and s0PC.
In `@src/cpu/pred/btb/ras.hh`:
- Around line 101-112: The UNIT_TEST conditional for the commitBranch
declaration uses the old type FetchStream which was renamed to FetchTarget;
update the `#ifdef` UNIT_TEST branch so commitBranch takes const FetchTarget
&stream (matching the non-UNIT_TEST signature) and keep the DynInstPtr parameter
unchanged; ensure the symbol commitBranch now has the same parameter types
(const FetchTarget &stream, const DynInstPtr &inst) in both branches to avoid
compilation errors.
🧹 Nitpick comments (5)
src/cpu/pred/btb/common.hh (1)
1-2: Header guard naming inconsistent with FetchTarget refactor.The header guards still reference
STREAM_STRUCTwhile the file now definesFetchTargetinstead ofFetchStream. Consider updating for consistency:Proposed fix
-#ifndef __CPU_PRED_BTB_STREAM_STRUCT_HH__ -#define __CPU_PRED_BTB_STREAM_STRUCT_HH__ +#ifndef __CPU_PRED_BTB_COMMON_HH__ +#define __CPU_PRED_BTB_COMMON_HH__And at line 753:
-#endif // __CPU_PRED_BTB_STREAM_STRUCT_HH__ +#endif // __CPU_PRED_BTB_COMMON_HH__src/cpu/pred/btb/decoupled_bpred.hh (1)
166-190: Consider updating log wording to FTQ.Line 169 and Line 171 still print “FSQ … target” after the FTQ migration. Small clarity win to rename these log strings.
src/cpu/pred/btb/decoupled_bpred.cc (3)
109-112: Optional: Rename variables to align with FTQ terminology.These variable names still use the old
Fsqprefix while the rest of the file has been refactored to useFtq/Targetterminology:
commitFsqEntryHasInstsVectorlastPhaseFsqEntryNumCommittedInstDistcommitFsqEntryFetchedInstsVectorlastPhaseFsqEntryNumFetchedInstDistConsider renaming for consistency, or deferring to a follow-up PR.
167-172: Optional: Update comments to reflect FTQ terminology.The comments on lines 167 and 171 still reference "FSQ" while the code has been refactored to use FTQ terminology.
📝 Suggested comment updates
- if (validateFSQEnqueue()) { - // Create new FSQ entry with the current prediction + if (validateFSQEnqueue()) { + // Create new FTQ entry with the current prediction processNewPrediction(); - DPRINTF(Override, "FSQ entry enqueued, prediction state reset\n"); + DPRINTF(Override, "FTQ entry enqueued, prediction state reset\n");
654-687: Optional: Rename function and update debug messages to align with FTQ terminology.The function
validateFSQEnqueue()and its debug messages still reference "FSQ" while the code uses FTQ/target queue operations internally.📝 Suggested renames
bool -DecoupledBPUWithBTB::validateFSQEnqueue() +DecoupledBPUWithBTB::validateFTQEnqueue() { // Monitor FSQ size for statistics dbpBtbStats.fsqEntryDist.sample(fetchTargetQueue.size(), 1); if (targetQueueFull()) { dbpBtbStats.fsqFullCannotEnq++; - DPRINTF(Override, "FSQ is full (%lu entries)\n", fetchTargetQueue.size()); + DPRINTF(Override, "FTQ is full (%lu entries)\n", fetchTargetQueue.size()); return false; } // 1. Check if a prediction is available to enqueue if (bpuState != BpuState::PREDICTION_OUTSTANDING) { - DPRINTF(Override, "No prediction available to enqueue into FSQ\n"); + DPRINTF(Override, "No prediction available to enqueue into FTQ\n"); return false; }
|
🚀 Performance test triggered: spec06-0.8c |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
There was a problem hiding this comment.
Actionable comments posted: 4
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/common.hh (1)
276-354: Newtidfield is not initialized in the constructor.The newly added
ThreadID tidfield at line 278 is not included in the constructor's initializer list (lines 325-349). This leavestidwith an indeterminate value, which could cause undefined behavior if accessed before being explicitly set.🐛 Proposed fix
FetchTarget() - : startPC(0), + : tid(0), + startPC(0), predTaken(false),
🤖 Fix all issues with AI agents
In `@src/cpu/o3/fetch.cc`:
- Around line 1542-1546: The code narrows entry.resolvedFTQId (a uint64_t) into
an unsigned int, risking truncation; update the declaration of stream_id in the
resolveQueue handling to preserve full width (e.g., use auto or uint64_t for
stream_id) where resolveQueue, entry, resolvedFTQId, and
dbpbtb->prepareResolveUpdateEntries are used so prepareResolveUpdateEntries
receives the full-width ID without truncation.
In `@src/cpu/pred/btb/docs/decoupled_bpred.md`:
- Line 98: Rename the documented field declaration that currently reads
"FetchTargetId fsqID;" to use the FTQ naming convention: change the identifier
to "ftqId" so it becomes "FetchTargetId ftqId;"; update any nearby references in
the same doc block to match ftqId to keep terminology consistent with the FTQ
migration and the FetchTargetId type.
- Around line 109-110: Update the outdated comment that refers to "fetch streams
(FSQ)" to use FTQ/Fetch Target terminology: change the second bullet describing
std::map<FetchTargetId, FetchTarget> FetchTargetQueue to say it manages fetch
targets (FTQ) or Fetch Target Queue, ensuring consistency with FetchTargetQueue,
FetchTargetId, and FetchTarget identifiers and removing the FSQ term.
- Around line 214-219: The FSQ enqueue stage text uses outdated FSQ terminology
(fsqId, FSQ); update the wording in the tryEnqFetchTarget section to match the
FetchTarget migration by replacing FSQ/`fsqId` mentions with the current names
(e.g., FetchTargetQueue identifiers or whatever the new stream id is), keep
references to `tryEnqFetchTarget`, `FetchTarget`, `FetchTargetQueue`, and
`receivedPred` intact, and adjust the bullet points to say "assigns stream id"
or the new identifier name and "increments the stream id for the next stream"
instead of "Increments `fsqId`."
🧹 Nitpick comments (1)
src/cpu/pred/btb/btb_tage.hh (1)
136-137: Minor formatting: missing space after comma.There's a missing space between
entry,andintin the parameter list.Suggested fix
void recoverPHist(const boost::dynamic_bitset<> &history, - const FetchTarget &entry,int shamt, bool cond_taken) override; + const FetchTarget &entry, int shamt, bool cond_taken) override;
|
[Generated by GEM5 Performance Robot] Ideal BTB PerformanceOverall Score
|
3 participants
Summary by CodeRabbit
Refactor
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.