Prefetch sample adjust#902
Conversation
Add per-source prefetch admission control with adaptive PFBad feedback and generated PrefetchSourceType names for stats. Enable the control path from the SMT Python config while leaving exploration scripts uncommitted. Change-Id: I002f06049555180409c148c11c06abb91d05ef67
Change-Id: Ied73acdc20d072925567d6cdf855f67183dce4fa
📝 WalkthroughWalkthroughThis PR adds a comprehensive prefetch admission control system to GEM5's prefetching framework, enabling windowed and online-adaptive admission control for multiple prefetch sources. The implementation spans the request layer, cache integration, prefetcher base interface, queued prefetcher logic, and configuration infrastructure. ChangesPrefetch Admission Control Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/mem/cache/prefetch/base.ccsrc/mem/cache/prefetch/base.cc:46:10: fatal error: 'mem/cache/prefetch/base.hh' file not found ... [truncated 1104 characters] ... tall/lib/clang/18/include" src/mem/cache/base.ccsrc/mem/cache/base.cc:46:10: fatal error: 'mem/cache/base.hh' file not found ... [truncated 1059 characters] ... clang/install/lib/clang/18/include" src/mem/cache/prefetch/composite_with_worker.ccsrc/mem/cache/prefetch/composite_with_worker.cc:1:10: fatal error: 'mem/cache/prefetch/composite_with_worker.hh' file not found ... [truncated 1213 characters] ... lude"
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mem/request.hh (1)
1220-1225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep cloned
Requestobjects consistent with the new PF metadata mirror.These setters now synchronize
_xsMetadatawithpfSource/pfDepth, butRequest(const Request& other)on Line 613 still copies only_xsMetadata.splitOnVaddr()on Line 744 uses that copy path, so a split/cloned request can carry a non-defaultXsMetadatawhilegetPFSource()/getPFDepth()fall back toPF_NONE/0. That leaves downstream code dependent on which accessor it reads.Suggested fix
Request(const Request& other) : _paddr(other._paddr), _size(other._size), _byteEnable(other._byteEnable), _requestorId(other._requestorId), _flags(other._flags), _cacheCoherenceFlags(other._cacheCoherenceFlags), privateFlags(other.privateFlags), _time(other._time), _taskId(other._taskId), _vaddr(other._vaddr), _extraData(other._extraData), _contextId(other._contextId), _pc(other._pc), _reqInstSeqNum(other._reqInstSeqNum), _xsMetadata(other._xsMetadata), _localAccessor(other._localAccessor), translateDelta(other.translateDelta), - accessDelta(other.accessDelta), depth(other.depth) + accessDelta(other.accessDelta), depth(other.depth), + pfSource(other.pfSource), + pfDepth(other.pfDepth), + firstReqAfterSquash(other.firstReqAfterSquash) { atomicOpFunctor.reset(other.atomicOpFunctor ? other.atomicOpFunctor->clone() : nullptr); }Also applies to: 1339-1354
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mem/request.hh` around lines 1220 - 1225, The copy path for cloned Requests leaves pfSource/pfDepth out of sync with _xsMetadata: update Request(const Request& other) (and any other clone/copy paths around the second region) to copy other._xsMetadata, set privateFlags VALID_XS_METADATA, and assign pfSource = other._xsMetadata.prefetchSource and pfDepth = other._xsMetadata.prefetchDepth so getPFSource()/getPFDepth() reflect the mirrored PF metadata (splitOnVaddr() uses the copy ctor so this will fix split clones as well).
🧹 Nitpick comments (3)
configs/example/smt_idealkmhv3.py (1)
8-8: 💤 Low valueConsider adding a public API to reset the config cache.
Directly accessing the private
_LOADED_PF_CONTROL_CONFIGvariable (line 29) couples this example script to implementation details ofPrefetcherConfig. If the caching mechanism changes, this code will break silently.♻️ Suggested approach
Add a public function in
PrefetcherConfig.py:def reset_pf_control_config(): """Clear cached config so next load picks up any PF_CONTROL_CONFIG changes.""" global _LOADED_PF_CONTROL_CONFIG _LOADED_PF_CONTROL_CONFIG = NoneThen in
smt_idealkmhv3.py:PrefetcherConfig.PF_CONTROL_CONFIG = { ... } - PrefetcherConfig._LOADED_PF_CONTROL_CONFIG = None + PrefetcherConfig.reset_pf_control_config()Also applies to: 13-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@configs/example/smt_idealkmhv3.py` at line 8, The example imports PrefetcherConfig but directly manipulates the private _LOADED_PF_CONTROL_CONFIG; add a public reset API in PrefetcherConfig (e.g., reset_pf_control_config()) that clears the internal cache, then update this example (smt_idealkmhv3.py) to call PrefetcherConfig.reset_pf_control_config() instead of touching _LOADED_PF_CONTROL_CONFIG; ensure the new function sets the module's cache variable to None so subsequent PrefetcherConfig.load/get calls pick up changes.src/mem/cache/prefetch/worker.cc (1)
68-97: ⚡ Quick winConsider extracting common admission control logic.
Both the
queueFilterenabled and disabled branches now duplicate the same admission control check and drop logic. The only difference is the queue-duplicate checks in the enabled branch.♻️ Suggested refactor to reduce duplication
auto dpp_it = localBuffer.begin(); while (count < depth && !localBuffer.empty()) { auto drop_local_packet = [&dpp_it]() { if (dpp_it->pkt != nullptr) { delete dpp_it->pkt; dpp_it->pkt = nullptr; } }; + + bool should_drop = false; if (queueFilter) { if (alreadyInQueue(pfq, dpp_it->pfInfo, dpp_it->priority)) { - drop_local_packet(); + should_drop = true; DPRINTF(WorkerPref, "Worker: [%lx, %d] was already in pfq\n", dpp_it->pfInfo.getAddr(), dpp_it->pfahead_host); } else if (alreadyInQueue(pfqMissingTranslation, dpp_it->pfInfo, dpp_it->priority)) { - drop_local_packet(); + should_drop = true; DPRINTF(WorkerPref, "Worker: [%lx, %d] was already in pfq\n", dpp_it->pfInfo.getAddr(), dpp_it->pfahead_host); - } else if (!admitPfControlDeferredPacket(*dpp_it)) { - drop_local_packet(); - DPRINTF(WorkerPref, "Worker: [%lx, %d] dropped by PF control admission\n", - dpp_it->pfInfo.getAddr(), dpp_it->pfahead_host); - } else { - addToQueue(pfq, *dpp_it); - DPRINTF(WorkerPref, "Worker: put [%lx, %d] into local pfq\n", dpp_it->pfInfo.getAddr(), - dpp_it->pfahead_host); } - } else { + } + + if (!should_drop) { if (!admitPfControlDeferredPacket(*dpp_it)) { - drop_local_packet(); + should_drop = true; DPRINTF(WorkerPref, "Worker: [%lx, %d] dropped by PF control admission\n", dpp_it->pfInfo.getAddr(), dpp_it->pfahead_host); - } else { - addToQueue(pfq, *dpp_it); - DPRINTF(WorkerPref, "Worker: put [%lx, %d] into local pfq\n", dpp_it->pfInfo.getAddr(), - dpp_it->pfahead_host); } } + + if (should_drop) { + drop_local_packet(); + } else { + addToQueue(pfq, *dpp_it); + DPRINTF(WorkerPref, "Worker: put [%lx, %d] into local pfq\n", dpp_it->pfInfo.getAddr(), + dpp_it->pfahead_host); + } + dpp_it = localBuffer.erase(dpp_it);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mem/cache/prefetch/worker.cc` around lines 68 - 97, The two branches guarded by queueFilter duplicate the same PF admission control and enqueue/drop behavior — extract the common admission check into a single path: first, if queueFilter perform the duplicate checks (alreadyInQueue against pfq and pfqMissingTranslation using dpp_it->pfInfo and dpp_it->priority) and drop if found; otherwise run the common admitPfControlDeferredPacket(*dpp_it) check once and call drop_local_packet() + DPRINTF on failure or addToQueue(pfq, *dpp_it) + DPRINTF on success. Refactor so the admitPfControlDeferredPacket, drop_local_packet, addToQueue, and corresponding DPRINTF calls are not duplicated, keeping only the duplicate queue-filtering calls inside the queueFilter-specific section and then delegating to the shared admission/enqueue logic.src/mem/cache/base.cc (1)
961-975: 💤 Low valueMinor redundant assignment when creating new XsMetadata.
When
pkt->req->hasXsMetadata()is false, lines 968-971 construct a newXsMetadatawith(pf_source, pf_depth), then lines 972-973 immediately reassign the same values. While harmless, this is slightly inefficient.♻️ Optional refactor to eliminate redundant assignment
- Request::XsMetadata xs_meta = - pkt->req->hasXsMetadata() ? - pkt->req->getXsMetadata() : - Request::XsMetadata(pf_source, pf_depth); - xs_meta.prefetchSource = pf_source; - xs_meta.prefetchDepth = pf_depth; + Request::XsMetadata xs_meta = pkt->req->hasXsMetadata() + ? pkt->req->getXsMetadata() + : Request::XsMetadata(); + xs_meta.prefetchSource = pf_source; + xs_meta.prefetchDepth = pf_depth;Or if the XsMetadata constructor is meant to initialize these fields:
- Request::XsMetadata xs_meta = - pkt->req->hasXsMetadata() ? - pkt->req->getXsMetadata() : - Request::XsMetadata(pf_source, pf_depth); - xs_meta.prefetchSource = pf_source; - xs_meta.prefetchDepth = pf_depth; + Request::XsMetadata xs_meta = pkt->req->getXsMetadata(); + if (!pkt->req->hasXsMetadata()) { + xs_meta = Request::XsMetadata(pf_source, pf_depth); + } else { + xs_meta.prefetchSource = pf_source; + xs_meta.prefetchDepth = pf_depth; + }Note: Only worthwhile if this code path is performance-sensitive.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mem/cache/base.cc` around lines 961 - 975, The code in the pure_prefetch_fill branch redundantly constructs Request::XsMetadata(pf_source, pf_depth) then immediately overwrites xs_meta.prefetchSource and xs_meta.prefetchDepth; update the logic around pkt->req->hasXsMetadata() so that when there is no existing XsMetadata you create the XsMetadata already initialized with pf_source and pf_depth and skip the subsequent reassignments, and only set xs_meta.prefetchSource/prefetchDepth when you created a default object without constructor args or when pkt->req->hasXsMetadata() is true; ensure you still call pkt->req->setXsMetadata(xs_meta) and keep the existing pkt->req->setPFSource(pf_source) / setPFDepth(pf_depth) calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configs/common/PrefetcherConfig.py`:
- Around line 218-251: The function _parse_source_pct_table returns early for
full-list input but fails to return the populated table for
dict/string/partial-list inputs, causing callers like _apply_pf_control to
receive None for prefetcher.pf_control_source_admit_pcts; fix this by adding a
final "return table" at the end of _parse_source_pct_table (after the for-loop)
so the populated list is always returned for all valid spec types, ensuring a
proper list of ints is passed to the C++ side.
In `@src/mem/cache/base.cc`:
- Around line 2253-2255: The new third parameter prefetch_fill_source on
BaseCache::allocateBlock is breaking existing two-argument calls; to fix, add a
default value for the parameter in the function declaration (and match it in the
definition if needed) so existing callers like allocateBlock(pkt, writebacks)
compile—use a sensible default such as PrefetchSourceType::None (or the
equivalent enum value used elsewhere); alternatively, update every call site
(e.g., the calls currently invoking allocateBlock(pkt, writebacks)) to pass an
explicit PrefetchSourceType argument if a non-default behavior is required.
- Around line 2234-2243: When prefetch_fill_source !=
PrefetchSourceType::PF_NONE the code must guarantee metadata is present before
reading prefetched depth; add a defensive assertion that pkt->req is non-null
and pkt->req->hasXsMetadata() is true (e.g. assert(pkt->req &&
pkt->req->hasXsMetadata())) right before reading
pkt->req->getXsMetadata().prefetchDepth so blk_meta.prefetchDepth cannot remain
stale from blk->getXsMetadata(); keep the existing assignment of
blk_meta.prefetchSource and continue to set blk_meta.prefetchDepth from
pkt->req->getXsMetadata().prefetchDepth after the assertion.
In `@src/mem/cache/prefetch/queued.cc`:
- Around line 383-389: The warmup branch unconditionally returns 100%, bypassing
configured admit pcts; change the early-return so that when
pfAdaptiveSampleCount < pfAdaptiveWarmupWindows you compute and return the
configured fallback/override admit percent for the given source instead of 100.
Specifically, inside the pfAdaptive && isPfAdaptiveLevel() block use
sanitizePfControlSource(source) to get source_idx and then consult per-source
settings (pfControlCurrentAdmitPctBySource[source_idx] or the controller-level
pf_control_admit_pct/pf_control_source_admit_pcts fallback logic) so the same
sweep/warmup policy is applied during adaptive warmup rather than forcing 100%.
- Around line 778-789: The code currently computes allowed/admit using
per-source counters unconditionally; change it so if this source is configured
to "use the global prefetch control action" (detect the fallback case for the
source rather than assuming per-source pct) you compute allowed and admit using
the global counters (pfControlWindowCandidates and pfControlWindowAdmitted) and
the global fallback pct, otherwise keep the existing per-source computation
using pfControlWindowCandidatesBySource[source_idx] and
pfControlWindowAdmittedBySource[source_idx]; update the branch around
sanitizePfControlSource / pfControlCurrentAdmitPctBySource[source_idx] so the
fallback path uses pfControlWindowCandidates, pfControlWindowAdmitted and the
global pct, while explicit per-source or adaptive modes continue to use the
per-source counters.
---
Outside diff comments:
In `@src/mem/request.hh`:
- Around line 1220-1225: The copy path for cloned Requests leaves
pfSource/pfDepth out of sync with _xsMetadata: update Request(const Request&
other) (and any other clone/copy paths around the second region) to copy
other._xsMetadata, set privateFlags VALID_XS_METADATA, and assign pfSource =
other._xsMetadata.prefetchSource and pfDepth = other._xsMetadata.prefetchDepth
so getPFSource()/getPFDepth() reflect the mirrored PF metadata (splitOnVaddr()
uses the copy ctor so this will fix split clones as well).
---
Nitpick comments:
In `@configs/example/smt_idealkmhv3.py`:
- Line 8: The example imports PrefetcherConfig but directly manipulates the
private _LOADED_PF_CONTROL_CONFIG; add a public reset API in PrefetcherConfig
(e.g., reset_pf_control_config()) that clears the internal cache, then update
this example (smt_idealkmhv3.py) to call
PrefetcherConfig.reset_pf_control_config() instead of touching
_LOADED_PF_CONTROL_CONFIG; ensure the new function sets the module's cache
variable to None so subsequent PrefetcherConfig.load/get calls pick up changes.
In `@src/mem/cache/base.cc`:
- Around line 961-975: The code in the pure_prefetch_fill branch redundantly
constructs Request::XsMetadata(pf_source, pf_depth) then immediately overwrites
xs_meta.prefetchSource and xs_meta.prefetchDepth; update the logic around
pkt->req->hasXsMetadata() so that when there is no existing XsMetadata you
create the XsMetadata already initialized with pf_source and pf_depth and skip
the subsequent reassignments, and only set xs_meta.prefetchSource/prefetchDepth
when you created a default object without constructor args or when
pkt->req->hasXsMetadata() is true; ensure you still call
pkt->req->setXsMetadata(xs_meta) and keep the existing
pkt->req->setPFSource(pf_source) / setPFDepth(pf_depth) calls.
In `@src/mem/cache/prefetch/worker.cc`:
- Around line 68-97: The two branches guarded by queueFilter duplicate the same
PF admission control and enqueue/drop behavior — extract the common admission
check into a single path: first, if queueFilter perform the duplicate checks
(alreadyInQueue against pfq and pfqMissingTranslation using dpp_it->pfInfo and
dpp_it->priority) and drop if found; otherwise run the common
admitPfControlDeferredPacket(*dpp_it) check once and call drop_local_packet() +
DPRINTF on failure or addToQueue(pfq, *dpp_it) + DPRINTF on success. Refactor so
the admitPfControlDeferredPacket, drop_local_packet, addToQueue, and
corresponding DPRINTF calls are not duplicated, keeping only the duplicate
queue-filtering calls inside the queueFilter-specific section and then
delegating to the shared admission/enqueue logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e40a127-d275-44fd-b101-b327ded0146b
📒 Files selected for processing (17)
configs/common/PrefetcherConfig.pyconfigs/example/smt_idealkmhv3.pysrc/mem/Request.pysrc/mem/SConscriptsrc/mem/cache/base.ccsrc/mem/cache/base.hhsrc/mem/cache/prefetch/Prefetcher.pysrc/mem/cache/prefetch/base.ccsrc/mem/cache/prefetch/base.hhsrc/mem/cache/prefetch/cdp.ccsrc/mem/cache/prefetch/composite_with_worker.ccsrc/mem/cache/prefetch/forwarder.ccsrc/mem/cache/prefetch/forwarder.hhsrc/mem/cache/prefetch/queued.ccsrc/mem/cache/prefetch/queued.hhsrc/mem/cache/prefetch/worker.ccsrc/mem/request.hh
| def _parse_source_pct_table(spec, path): | ||
| table = [-1] * len(PF_SOURCE_NAMES) | ||
| if spec in (None, "", {}, []): | ||
| return [] | ||
|
|
||
| if isinstance(spec, dict): | ||
| items = spec.items() | ||
| elif isinstance(spec, str): | ||
| items = [] | ||
| for raw_pair in spec.split(','): | ||
| pair = raw_pair.strip() | ||
| if not pair: | ||
| continue | ||
| if '=' not in pair: | ||
| _config_error(path, "source=pct entries", pair) | ||
| raw_source, raw_pct = pair.split('=', 1) | ||
| items.append((raw_source.strip(), raw_pct.strip())) | ||
| elif isinstance(spec, (list, tuple)): | ||
| if len(spec) == len(PF_SOURCE_NAMES): | ||
| for idx, pct in enumerate(spec): | ||
| table[idx] = _validate_source_pct(pct, f"{path}.{idx}") | ||
| return table | ||
| items = spec | ||
| else: | ||
| _config_error(path, "a dict, full list, pair list, or string", spec) | ||
|
|
||
| for item in items: | ||
| try: | ||
| raw_source, raw_pct = item | ||
| except (TypeError, ValueError): | ||
| _config_error(path, "source/pct pairs", item) | ||
| source = _parse_pf_source(str(raw_source).strip()) | ||
| table[source] = _validate_source_pct(raw_pct, f"{path}.{raw_source}") | ||
| return table |
There was a problem hiding this comment.
Missing return table statement causes function to return None.
When spec is a dict, string, or partial list, the function parses items and populates table in the for loop (lines 244-250), but never returns it. This causes _apply_pf_control at line 312 to assign None to prefetcher.pf_control_source_admit_pcts, which will likely cause runtime errors when the C++ side expects a vector of integers.
🐛 Proposed fix
for item in items:
try:
raw_source, raw_pct = item
except (TypeError, ValueError):
_config_error(path, "source/pct pairs", item)
source = _parse_pf_source(str(raw_source).strip())
table[source] = _validate_source_pct(raw_pct, f"{path}.{raw_source}")
+ return table🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@configs/common/PrefetcherConfig.py` around lines 218 - 251, The function
_parse_source_pct_table returns early for full-list input but fails to return
the populated table for dict/string/partial-list inputs, causing callers like
_apply_pf_control to receive None for prefetcher.pf_control_source_admit_pcts;
fix this by adding a final "return table" at the end of _parse_source_pct_table
(after the for-loop) so the populated list is always returned for all valid spec
types, ensuring a proper list of ints is passed to the C++ side.
| if (prefetch_fill_source != PrefetchSourceType::PF_NONE) { | ||
| blk_meta.prefetchSource = prefetch_fill_source; | ||
| if (pkt->req && pkt->req->hasXsMetadata()) { | ||
| blk_meta.prefetchDepth = | ||
| pkt->req->getXsMetadata().prefetchDepth; | ||
| } | ||
| } else { | ||
| blk_meta.prefetchSource = PrefetchSourceType::PF_NONE; | ||
| blk_meta.prefetchDepth = 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add defensive assertion for metadata presence during prefetch fills.
When prefetch_fill_source != PF_NONE (indicating a prefetch fill), the code sets blk_meta.prefetchSource but only sets blk_meta.prefetchDepth if pkt->req->hasXsMetadata() returns true. For newly allocated blocks, if the packet lacks metadata, prefetchDepth would be left at whatever blk->getXsMetadata() returned (potentially uninitialized or stale from the evicted block).
The upstream code (lines 963-975) ensures metadata is set for pure prefetch fills, so this shouldn't occur in practice. However, an assertion would make this invariant explicit and catch violations during development.
🛡️ Proposed defensive assertion
Request::XsMetadata blk_meta = blk->getXsMetadata();
if (prefetch_fill_source != PrefetchSourceType::PF_NONE) {
blk_meta.prefetchSource = prefetch_fill_source;
if (pkt->req && pkt->req->hasXsMetadata()) {
blk_meta.prefetchDepth =
pkt->req->getXsMetadata().prefetchDepth;
+ } else {
+ // Pure prefetch fills should always have metadata set by line 963-975
+ panic("Prefetch fill missing XsMetadata: source=%d, addr=%#x",
+ prefetch_fill_source, pkt->getAddr());
}
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (prefetch_fill_source != PrefetchSourceType::PF_NONE) { | |
| blk_meta.prefetchSource = prefetch_fill_source; | |
| if (pkt->req && pkt->req->hasXsMetadata()) { | |
| blk_meta.prefetchDepth = | |
| pkt->req->getXsMetadata().prefetchDepth; | |
| } | |
| } else { | |
| blk_meta.prefetchSource = PrefetchSourceType::PF_NONE; | |
| blk_meta.prefetchDepth = 0; | |
| } | |
| if (prefetch_fill_source != PrefetchSourceType::PF_NONE) { | |
| blk_meta.prefetchSource = prefetch_fill_source; | |
| if (pkt->req && pkt->req->hasXsMetadata()) { | |
| blk_meta.prefetchDepth = | |
| pkt->req->getXsMetadata().prefetchDepth; | |
| } else { | |
| // Pure prefetch fills should always have metadata set by line 963-975 | |
| panic("Prefetch fill missing XsMetadata: source=%d, addr=%#x", | |
| prefetch_fill_source, pkt->getAddr()); | |
| } | |
| } else { | |
| blk_meta.prefetchSource = PrefetchSourceType::PF_NONE; | |
| blk_meta.prefetchDepth = 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mem/cache/base.cc` around lines 2234 - 2243, When prefetch_fill_source !=
PrefetchSourceType::PF_NONE the code must guarantee metadata is present before
reading prefetched depth; add a defensive assertion that pkt->req is non-null
and pkt->req->hasXsMetadata() is true (e.g. assert(pkt->req &&
pkt->req->hasXsMetadata())) right before reading
pkt->req->getXsMetadata().prefetchDepth so blk_meta.prefetchDepth cannot remain
stale from blk->getXsMetadata(); keep the existing assignment of
blk_meta.prefetchSource and continue to set blk_meta.prefetchDepth from
pkt->req->getXsMetadata().prefetchDepth after the assertion.
| BaseCache::allocateBlock( | ||
| const PacketPtr pkt, PacketList &writebacks, | ||
| PrefetchSourceType prefetch_fill_source) |
There was a problem hiding this comment.
Compilation error: existing callers not updated with new parameter.
The allocateBlock signature now requires a third parameter prefetch_fill_source, but existing callers at lines 1912 and 1992 still call it with only two arguments:
blk = allocateBlock(pkt, writebacks);These calls will fail to compile unless a default parameter value is added or all call sites are updated.
🔧 Proposed fix options
Option 1: Add default parameter value
CacheBlk*
BaseCache::allocateBlock(
const PacketPtr pkt, PacketList &writebacks,
- PrefetchSourceType prefetch_fill_source)
+ PrefetchSourceType prefetch_fill_source = PrefetchSourceType::PF_NONE)Option 2: Update all call sites
At lines 1912 and 1992:
-blk = allocateBlock(pkt, writebacks);
+blk = allocateBlock(pkt, writebacks, PrefetchSourceType::PF_NONE);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mem/cache/base.cc` around lines 2253 - 2255, The new third parameter
prefetch_fill_source on BaseCache::allocateBlock is breaking existing
two-argument calls; to fix, add a default value for the parameter in the
function declaration (and match it in the definition if needed) so existing
callers like allocateBlock(pkt, writebacks) compile—use a sensible default such
as PrefetchSourceType::None (or the equivalent enum value used elsewhere);
alternatively, update every call site (e.g., the calls currently invoking
allocateBlock(pkt, writebacks)) to pass an explicit PrefetchSourceType argument
if a non-default behavior is required.
| if (pfAdaptive && isPfAdaptiveLevel()) { | ||
| const unsigned source_idx = sanitizePfControlSource(source); | ||
| if (pfAdaptiveSampleCount < pfAdaptiveWarmupWindows) { | ||
| return 100; | ||
| } | ||
| return pfControlCurrentAdmitPctBySource[source_idx]; | ||
| } |
There was a problem hiding this comment.
Adaptive warmup ignores the configured fallback/override policy.
While pfAdaptiveSampleCount < pfAdaptiveWarmupWindows, this forces every source to 100% admission. That bypasses pf_control_admit_pct, pf_control_source_admit_pcts, and the sweep/warmup policy defined for the controller, so the first adaptive samples are collected under a different policy than the one the config requested.
Suggested fix
if (pfAdaptive && isPfAdaptiveLevel()) {
const unsigned source_idx = sanitizePfControlSource(source);
- if (pfAdaptiveSampleCount < pfAdaptiveWarmupWindows) {
- return 100;
+ if (pfAdaptiveSampleCount >= pfAdaptiveWarmupWindows) {
+ return pfControlCurrentAdmitPctBySource[source_idx];
}
- return pfControlCurrentAdmitPctBySource[source_idx];
}
if (window_index >= pfControlWarmupWindows) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mem/cache/prefetch/queued.cc` around lines 383 - 389, The warmup branch
unconditionally returns 100%, bypassing configured admit pcts; change the
early-return so that when pfAdaptiveSampleCount < pfAdaptiveWarmupWindows you
compute and return the configured fallback/override admit percent for the given
source instead of 100. Specifically, inside the pfAdaptive &&
isPfAdaptiveLevel() block use sanitizePfControlSource(source) to get source_idx
and then consult per-source settings
(pfControlCurrentAdmitPctBySource[source_idx] or the controller-level
pf_control_admit_pct/pf_control_source_admit_pcts fallback logic) so the same
sweep/warmup policy is applied during adaptive warmup rather than forcing 100%.
| const unsigned source_idx = sanitizePfControlSource(source); | ||
| const unsigned pct = pfControlCurrentAdmitPctBySource[source_idx]; | ||
| statsQueued.pfControlCandidates++; | ||
| statsQueued.pfControlCandidatesByPct[pct]++; | ||
| statsQueued.pfControlCandidatesBySource[source_idx]++; | ||
| pfControlWindowCandidates++; | ||
| pfControlWindowCandidatesBySource[source_idx]++; | ||
|
|
||
| const uint64_t allowed = | ||
| (pfControlWindowCandidatesBySource[source_idx] * pct + 99) / 100; | ||
| const bool admit = | ||
| allowed > pfControlWindowAdmittedBySource[source_idx]; |
There was a problem hiding this comment.
The global fallback percentage is being enforced per source, not globally.
For sources whose config is -1 ("use the global prefetch control action"), allowed is still derived from pfControlWindowCandidatesBySource[source_idx]. That gives each active source its own independent budget. For example, with a 10% fallback and one candidate from eight sources in the same window, all eight are admitted. That breaks the documented semantics of the global fallback action.
The fix needs to use pfControlWindowCandidates / pfControlWindowAdmitted on the fallback path, and reserve the per-source counters for explicit per-source overrides or adaptive mode.
Suggested direction
const unsigned source_idx = sanitizePfControlSource(source);
const unsigned pct = pfControlCurrentAdmitPctBySource[source_idx];
+ const bool use_per_source_budget =
+ (pfAdaptive && isPfAdaptiveLevel()) ||
+ pfControlSourceAdmitPct[source_idx] >= 0;
+
+ auto &window_candidates = use_per_source_budget ?
+ pfControlWindowCandidatesBySource[source_idx] :
+ pfControlWindowCandidates;
+ auto &window_admitted = use_per_source_budget ?
+ pfControlWindowAdmittedBySource[source_idx] :
+ pfControlWindowAdmitted;
+
statsQueued.pfControlCandidates++;
statsQueued.pfControlCandidatesByPct[pct]++;
statsQueued.pfControlCandidatesBySource[source_idx]++;
pfControlWindowCandidates++;
pfControlWindowCandidatesBySource[source_idx]++;
- const uint64_t allowed =
- (pfControlWindowCandidatesBySource[source_idx] * pct + 99) / 100;
- const bool admit =
- allowed > pfControlWindowAdmittedBySource[source_idx];
+ const uint64_t allowed = (window_candidates * pct + 99) / 100;
+ const bool admit = allowed > window_admitted;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mem/cache/prefetch/queued.cc` around lines 778 - 789, The code currently
computes allowed/admit using per-source counters unconditionally; change it so
if this source is configured to "use the global prefetch control action" (detect
the fallback case for the source rather than assuming per-source pct) you
compute allowed and admit using the global counters (pfControlWindowCandidates
and pfControlWindowAdmitted) and the global fallback pct, otherwise keep the
existing per-source computation using
pfControlWindowCandidatesBySource[source_idx] and
pfControlWindowAdmittedBySource[source_idx]; update the branch around
sanitizePfControlSource / pfControlCurrentAdmitPctBySource[source_idx] so the
fallback path uses pfControlWindowCandidates, pfControlWindowAdmitted and the
global pct, while explicit per-source or adaptive modes continue to use the
per-source counters.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
1 participant
Summary by CodeRabbit
Release Notes