Conversation
|
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:
📝 WalkthroughWalkthroughAdds VTAGE value prediction support across metadata, o3 pipeline wiring, build/config registration, and the VTAGE implementation. It also updates the PGO helper script to rebuild without PGO when profiling or profile merging fails. ChangesVTAGE value predictor integration
PGO fallback script
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (4)
src/cpu/valuepred/vtage_metadata.hh (1)
41-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument the lifetime contract of
fetchTarget.
VPHistoryRequestExtholds a raw, non-owningfetchTargetpointer into an FTQ entry (populated inFetch::processSingleInstructionvia&dbpbtb->ftqFetchingTarget(tid)). Current usage is safe because it's dereferenced synchronously inside the samevaluePredict()call, but nothing in the header states this constraint. A future caller that stashes the extension/record for later (e.g. across cycles) could dereference a stale/reused FTQ slot.Consider adding a short comment documenting that
fetchTargetis only valid for the duration of thevaluePredict()call that receives this extension.🤖 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/cpu/valuepred/vtage_metadata.hh` around lines 41 - 51, `VPHistoryRequestExt` stores a raw non-owning `fetchTarget` pointer whose validity is only synchronous, but that constraint is not documented. Add a brief lifetime comment on `fetchTarget` in `VPHistoryRequestExt` (and, if needed, near the constructor) stating that it points to an FTQ entry and is only valid for the duration of the `valuePredict()` call that consumes this extension. Use the `VPHistoryRequestExt` and `fetchTarget` symbols so future callers know not to retain it across cycles or stash it beyond the immediate call.src/cpu/valuepred/vtage.hh (2)
52-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNaming convention: constant and method names don't follow the repo's lower_snake_case/ALL_CAPS rules.
ValueWays(Line 53) is UpperCamelCase rather than ALL_CAPS, and the private helper methods declared here (bitMask,maxConf,mix64,foldHistory,fillIndicesAndTags, etc.) are camelCase rather than lower_snake_case. Note several public overrides (predict,update,specUpdate,squash,getValuePredictorType) are constrained by the baseVPUnitinterface and can't be renamed independently, so full compliance would require broader base-class changes; treat this as advisory only for the new private helpers.As per coding guidelines, "Types and classes should use UpperCamelCase naming convention. Functions and methods should use lower_snake_case naming convention. Constants should use ALL_CAPS naming convention."
Also applies to: 184-213
🤖 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/cpu/valuepred/vtage.hh` around lines 52 - 53, The new constant and helper names in VTAGE violate the repo naming rules: ValueWays should be ALL_CAPS, and the private helper methods in VTAGE should use lower_snake_case. Update the identifiers in VTAGE to match the convention (for example, the constant definition and helpers like bitMask, maxConf, mix64, foldHistory, and fillIndicesAndTags), while leaving the VPUnit override methods alone since they follow the inherited interface and should not be renamed independently.Source: Coding guidelines
93-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deriving
numBanksfromhistLengths.size()instead of a separately configurable param.
numHistories/numBanks/histLengthsare three independent Params that must satisfynumBanks == numHistories + 1andhistLengths.size() == numBanks(enforced only via runtimegem5_assertin the constructor). A user could easily set inconsistent values and only discover it via a fatal assertion at simulation start. DerivingnumBanks(and evennumHistories) directly fromhistLengths.size()would remove this footgun.🤖 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/cpu/valuepred/vtage.hh` around lines 93 - 95, The VTAGE params currently expose independent numHistories, numBanks, and histLengths values, but numBanks should be derived from histLengths.size() so callers cannot configure inconsistent state. Update the VTAGE parameter handling in vtage.hh and the VTAGE constructor/initialization path to stop treating numBanks as a separate user-configurable source of truth, and instead compute it from histLengths (and, if possible, derive numHistories from that relationship as well). Make sure all uses of numBanks and numHistories in the VTAGE class refer to the derived values so the existing runtime gem5_assert checks are no longer needed as the primary validation.src/cpu/valuepred/vtage.cc (1)
184-201: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
foldHistoryis O(length) per call regardless of set-bit density; called 4× per bank infillIndicesAndTags.For every predict() call,
fillIndicesAndTagsinvokesfoldHistory4 times per bank (history/path history × index/tag), each iterating every bit up tolengthviahistory.test(bit). WithnumBanks=9and history lengths up to 127, this is a meaningful hot-path cost on every fetch-time value prediction.boost::dynamic_bitsetsupportsfind_first()/find_next()to skip directly to set bits, which would make the loop cost proportional to the population count rather than the full length.♻️ Sketch using find_next for sparse history
- for (size_t bit = 0; bit < limit; ++bit) { - if (!history.test(bit)) { - continue; - } - accum ^= mix64((static_cast<uint64_t>(bit) + 1) * - 0x9e3779b97f4a7c15ULL ^ salt); - accum = (accum << 7) | (accum >> (64 - 7)); - } + for (auto bit = history.find_first(); + bit != boost::dynamic_bitset<>::npos && bit < limit; + bit = history.find_next(bit)) { + accum ^= mix64((static_cast<uint64_t>(bit) + 1) * + 0x9e3779b97f4a7c15ULL ^ salt); + accum = (accum << 7) | (accum >> (64 - 7)); + }Also applies to: 204-238
🤖 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/cpu/valuepred/vtage.cc` around lines 184 - 201, The VTAGE::foldHistory helper currently scans every bit up to the requested length on each call, which makes the predict-time hot path in fillIndicesAndTags unnecessarily expensive. Update foldHistory to iterate only over set bits in the boost::dynamic_bitset history using find_first/find_next (while preserving the current mixing, salt, and bitMask(out_bits) behavior), so the cost scales with the number of set bits rather than the full history length. Make sure the change still works for both history and path-history inputs used by fillIndicesAndTags across all banks.
🤖 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 `@src/cpu/valuepred/vtage.cc`:
- Around line 350-372: The allocation logic in VTAGE::chooseAllocationStartBank
leaves bank 0 permanently unused, so update() and predict() never exercise the
base-bank path and the hit_bank == 0 branch is dead. Fix this by either making
bank 0 the explicit always-valid base predictor and updating the
allocation/prediction paths accordingly, or by removing the extra reserved bank
from the VTAGE bank layout and deleting the unreachable special-case in
chooseAllocationStartBank().
---
Nitpick comments:
In `@src/cpu/valuepred/vtage_metadata.hh`:
- Around line 41-51: `VPHistoryRequestExt` stores a raw non-owning `fetchTarget`
pointer whose validity is only synchronous, but that constraint is not
documented. Add a brief lifetime comment on `fetchTarget` in
`VPHistoryRequestExt` (and, if needed, near the constructor) stating that it
points to an FTQ entry and is only valid for the duration of the
`valuePredict()` call that consumes this extension. Use the
`VPHistoryRequestExt` and `fetchTarget` symbols so future callers know not to
retain it across cycles or stash it beyond the immediate call.
In `@src/cpu/valuepred/vtage.cc`:
- Around line 184-201: The VTAGE::foldHistory helper currently scans every bit
up to the requested length on each call, which makes the predict-time hot path
in fillIndicesAndTags unnecessarily expensive. Update foldHistory to iterate
only over set bits in the boost::dynamic_bitset history using
find_first/find_next (while preserving the current mixing, salt, and
bitMask(out_bits) behavior), so the cost scales with the number of set bits
rather than the full history length. Make sure the change still works for both
history and path-history inputs used by fillIndicesAndTags across all banks.
In `@src/cpu/valuepred/vtage.hh`:
- Around line 52-53: The new constant and helper names in VTAGE violate the repo
naming rules: ValueWays should be ALL_CAPS, and the private helper methods in
VTAGE should use lower_snake_case. Update the identifiers in VTAGE to match the
convention (for example, the constant definition and helpers like bitMask,
maxConf, mix64, foldHistory, and fillIndicesAndTags), while leaving the VPUnit
override methods alone since they follow the inherited interface and should not
be renamed independently.
- Around line 93-95: The VTAGE params currently expose independent numHistories,
numBanks, and histLengths values, but numBanks should be derived from
histLengths.size() so callers cannot configure inconsistent state. Update the
VTAGE parameter handling in vtage.hh and the VTAGE constructor/initialization
path to stop treating numBanks as a separate user-configurable source of truth,
and instead compute it from histLengths (and, if possible, derive numHistories
from that relationship as well). Make sure all uses of numBanks and numHistories
in the VTAGE class refer to the derived values so the existing runtime
gem5_assert checks are no longer needed as the primary validation.
🪄 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: c90987cb-2a53-4d5a-9f73-d79f6a4328e3
📒 Files selected for processing (12)
configs/example/idealkmhv3.pysrc/cpu/o3/commit.ccsrc/cpu/o3/dyn_inst.hhsrc/cpu/o3/fetch.ccsrc/cpu/o3/lsq.ccsrc/cpu/valuepred/SConscriptsrc/cpu/valuepred/ValuePredictor.pysrc/cpu/valuepred/composite_value_predictor.ccsrc/cpu/valuepred/valuepred_metadata.hhsrc/cpu/valuepred/vtage.ccsrc/cpu/valuepred/vtage.hhsrc/cpu/valuepred/vtage_metadata.hh
| unsigned | ||
| VTAGE::chooseAllocationStartBank(int hit_bank) | ||
| { | ||
| unsigned start_bank = 1; | ||
| if (hit_bank >= 0) { | ||
| start_bank = std::min<unsigned>(static_cast<unsigned>(hit_bank + 1), | ||
| numBanks - 1); | ||
| if (hit_bank == 0 && start_bank + 1 < numBanks) { | ||
| ++start_bank; | ||
| } | ||
| } | ||
|
|
||
| if (enableStochasticTraining && start_bank > 1 && | ||
| chance(shortHistoryAllocBias)) { | ||
| start_bank = 1; | ||
| } | ||
| if (enableStochasticTraining && start_bank + 1 < numBanks && | ||
| chance(deepAllocExtraHopProb)) { | ||
| ++start_bank; | ||
| } | ||
|
|
||
| return std::min(start_bank, numBanks - 1); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/cpu/valuepred/vtage.cc"
echo "== file info =="
wc -l "$file"
echo
echo "== outline =="
ast-grep outline "$file" --view expanded || true
echo
echo "== relevant ranges =="
sed -n '90,130p;330,390p;470,520p;590,630p' "$file" | cat -nRepository: OpenXiangShan/GEM5
Length of output: 8151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/cpu/valuepred/vtage.cc"
echo "== constructor/init section =="
sed -n '1,120p' "$file" | cat -n
echo
echo "== predict hit-scan and allocation sections =="
sed -n '450,520p;590,625p' "$file" | cat -n
echo
echo "== search for bank 0 specific logic =="
rg -n "bank 0|bank0|hitBank == 0|numBanks ==|numHistories|tables\\[.*\\]\\[0\\]|\\[0\\].*valid|start_bank" "$file"Repository: OpenXiangShan/GEM5
Length of output: 11336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=$(git ls-files 'src/cpu/valuepred/vtage.*' 'src/cpu/valuepred/*' 'src/**/valuepred/*' | tr '\n' ' ')
echo "== candidate files =="
printf '%s\n' "$files"
echo
echo "== bank 0 / valid / allocation writes =="
rg -n "valid\\s*=\\s*true|valid\\s*=\\s*false|chooseAllocationStartBank|tables\\[.*\\]\\[0\\]|bank == 0|hitBank == 0|base bank|untyped|untagged|always-valid|predict\\(|update\\(" src/cpu/valuepred src -g '!**/*.o'
echo
echo "== VTAGEEntry definition and defaults =="
fd -a 'vtage*.hh' src || true
fd -a 'vtage*.cc' src || trueRepository: OpenXiangShan/GEM5
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all vtage.cc validity writes =="
rg -n "valid\s*=" src/cpu/valuepred/vtage.cc
echo
echo "== bank-0 and base-bank references in VTAGE headers/metadata =="
rg -n "bank 0|base bank|bank0|hit_bank == 0|numBanks == numHistories \+ 1|histLengths\[0\]|tables\[.*\]\[0\]|valid = false|valid = true" \
src/cpu/valuepred/vtage.hh src/cpu/valuepred/vtage_metadata.hh src/cpu/valuepred/ValuePredictor.py src/cpu/valuepred/vtage.cc
echo
echo "== vtage.hh relevant structs/enums =="
sed -n '1,140p;180,250p' src/cpu/valuepred/vtage.hh | cat -nRepository: OpenXiangShan/GEM5
Length of output: 8792
Bank 0 is reserved but never populated
chooseAllocationStartBank() never returns 0, so update() never writes tables[tid][0], and predict() can never hit bank 0. That makes the hit_bank == 0 branch unreachable while still reserving a full base-bank slice via numBanks == numHistories + 1. If bank 0 is meant to be the always-valid base predictor, it needs explicit handling; otherwise remove the extra bank and the dead branch.
🤖 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/cpu/valuepred/vtage.cc` around lines 350 - 372, The allocation logic in
VTAGE::chooseAllocationStartBank leaves bank 0 permanently unused, so update()
and predict() never exercise the base-bank path and the hit_bank == 0 branch is
dead. Fix this by either making bank 0 the explicit always-valid base predictor
and updating the allocation/prediction paths accordingly, or by removing the
extra reserved bank from the VTAGE bank layout and deleting the unreachable
special-case in chooseAllocationStartBank().
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
happy-lx
force-pushed
the
lvp-vtage
branch
2 times, most recently
from
1d5c39c to
1eea5de
Compare
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
configs/example/idealkmhv3.py (1)
79-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant explicit arb assignment.
arb=CVPFixedPriorityArb()matches the class default already defined forCompositeValuePredictorin ValuePredictor.py. Harmless, but adds no behavioral change over omitting 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 `@configs/example/idealkmhv3.py` at line 79, The explicit arb assignment is redundant because CompositeValuePredictor already defaults to CVPFixedPriorityArb; remove the unnecessary arb=CVPFixedPriorityArb() argument from the CompositeValuePredictor configuration in the example setup so it relies on the class default.src/cpu/valuepred/ValuePredictor.py (1)
100-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate probability knobs stay in
[0, 1].These
Param.Floatvalues feedVTAGE::chance(), where values<= 0become never and>= 1become always. Add VTAGE constructor checks so config typos fail fast instead of silently changing training policy.🤖 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/cpu/valuepred/ValuePredictor.py` around lines 100 - 115, The probability parameters in ValuePredictor are currently unconstrained, so invalid config values can silently change VTAGE::chance() behavior. Add validation in the VTAGE constructor or initialization path that checks each probability knob from ValuePredictor (such as allocProbLoadL1Hit, allocProbLoadMiss, confIncProbLowValue, confIncProbFastLoad, uIncProbFastLoad, valueArrayUpgradeProb, shortHistoryAllocBias, and deepAllocExtraHopProb) is within [0, 1], and fail fast with a clear error if not.
🤖 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 `@src/cpu/valuepred/vtage.cc`:
- Around line 38-45: The constructor initialization in vtage.cc computes
bankSize, valueEntriesPerWay, and totalValueEntries with unchecked shifts before
the VTAGE assertions can run, so invalid params can cause undefined behavior
first. Move the bounds validation ahead of these member initializers or replace
the raw shifts with a checked/guarded helper in the VTAGE constructor so
logBankSize and logValueArrayEntries are verified before any array sizing
occurs.
---
Nitpick comments:
In `@configs/example/idealkmhv3.py`:
- Line 79: The explicit arb assignment is redundant because
CompositeValuePredictor already defaults to CVPFixedPriorityArb; remove the
unnecessary arb=CVPFixedPriorityArb() argument from the CompositeValuePredictor
configuration in the example setup so it relies on the class default.
In `@src/cpu/valuepred/ValuePredictor.py`:
- Around line 100-115: The probability parameters in ValuePredictor are
currently unconstrained, so invalid config values can silently change
VTAGE::chance() behavior. Add validation in the VTAGE constructor or
initialization path that checks each probability knob from ValuePredictor (such
as allocProbLoadL1Hit, allocProbLoadMiss, confIncProbLowValue,
confIncProbFastLoad, uIncProbFastLoad, valueArrayUpgradeProb,
shortHistoryAllocBias, and deepAllocExtraHopProb) is within [0, 1], and fail
fast with a clear error if not.
🪄 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: 3574ad79-a819-4427-994a-4c6ed42e4e1f
📒 Files selected for processing (12)
configs/example/idealkmhv3.pysrc/cpu/o3/commit.ccsrc/cpu/o3/dyn_inst.hhsrc/cpu/o3/fetch.ccsrc/cpu/o3/lsq.ccsrc/cpu/valuepred/SConscriptsrc/cpu/valuepred/ValuePredictor.pysrc/cpu/valuepred/composite_value_predictor.ccsrc/cpu/valuepred/valuepred_metadata.hhsrc/cpu/valuepred/vtage.ccsrc/cpu/valuepred/vtage.hhsrc/cpu/valuepred/vtage_metadata.hh
🚧 Files skipped from review as they are similar to previous changes (9)
- src/cpu/valuepred/composite_value_predictor.cc
- src/cpu/valuepred/valuepred_metadata.hh
- src/cpu/valuepred/SConscript
- src/cpu/o3/lsq.cc
- src/cpu/o3/commit.cc
- src/cpu/valuepred/vtage_metadata.hh
- src/cpu/o3/fetch.cc
- src/cpu/o3/dyn_inst.hh
- src/cpu/valuepred/vtage.hh
| logBankSize(params.logBankSize), | ||
| bankSize(1u << params.logBankSize), | ||
| tagBits(params.tagBits), | ||
| confBits(params.confBits), | ||
| usefulBits(params.usefulBits), | ||
| logValueArrayEntries(params.logValueArrayEntries), | ||
| valueEntriesPerWay(1u << params.logValueArrayEntries), | ||
| totalValueEntries(ValueWays * (1u << params.logValueArrayEntries)), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all VTAGE size shifts and the declared storage widths before patching.
set -euo pipefail
rg -n -C3 '1u\s*<<|logBankSize|logValueArrayEntries|bankSize|valueEntriesPerWay|totalValueEntries' src/cpu/valuepred/vtage.cc src/cpu/valuepred/vtage.hhRepository: OpenXiangShan/GEM5
Length of output: 8312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect VTAGE parameter definitions and any config-side bounds on the shift inputs.
rg -n -C3 'logBankSize|logValueArrayEntries|bankSize|valueEntriesPerWay|totalValueEntries|VTAGE' src include configs . \
-g '!**/build/**' -g '!**/dist/**' -g '!**/node_modules/**'Repository: OpenXiangShan/GEM5
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the constructor body to see whether any validation happens before/after sizing.
sed -n '1,140p' src/cpu/valuepred/vtage.ccRepository: OpenXiangShan/GEM5
Length of output: 5185
Guard the size shifts before constructing the arrays. 1u << params.logBankSize and 1u << params.logValueArrayEntries are evaluated in the initializer list, before the constructor assertions run, so a large config value can trigger undefined behavior before gem5 reports the bad parameter. Use a checked shift or validate the bounds first.
🤖 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/cpu/valuepred/vtage.cc` around lines 38 - 45, The constructor
initialization in vtage.cc computes bankSize, valueEntriesPerWay, and
totalValueEntries with unchecked shifts before the VTAGE assertions can run, so
invalid params can cause undefined behavior first. Move the bounds validation
ahead of these member initializers or replace the raw shifts with a
checked/guarded helper in the VTAGE constructor so logBankSize and
logValueArrayEntries are verified before any array sizing occurs.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
configs/example/idealkmhv3.py (1)
79-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant explicit
arbassignment.
CVPFixedPriorityArb()is already the default value forCompositeValuePredictor.arbperValuePredictor.py. Explicitly passing it here is harmless but unnecessary.🤖 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/idealkmhv3.py` at line 79, Remove the redundant explicit arb assignment where CompositeValuePredictor is configured in the example setup, since CVPFixedPriorityArb() is already the default for the arb parameter. Update the call site that passes arb=CVPFixedPriorityArb() so it relies on the default value instead, keeping the configuration cleaner without changing behavior.util/pgo/basic_pgo_new.sh (3)
35-35: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFallback exits 0 despite the original PGO build failing.
rebuild_without_pgoalwaysexit 0s, even when invoked because profiling or profdata merge failed. Callers (e.g., CI) will see a successful exit even though PGO instrumentation never completed, potentially masking a real problem in the PGO toolchain.🤖 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 `@util/pgo/basic_pgo_new.sh` at line 35, The fallback path in rebuild_without_pgo currently always returns success, which can hide a failed PGO build or profiling step. Update rebuild_without_pgo in basic_pgo_new.sh so it preserves and propagates the original failure status instead of unconditionally exiting 0; make the function return or exit with the caller’s non-zero code when it is invoked due to profiling or profdata merge errors.
84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a subshell over
cd ..to avoid needing to cd back.Minor robustness nit flagged by shellcheck.
🤖 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 `@util/pgo/basic_pgo_new.sh` at line 84, The script uses a plain cd .. in the PGO setup flow, which requires later restoring the directory and is less robust. Update the basic_pgo_new.sh logic to run that parent-directory command in a subshell instead of changing the caller’s working directory, keeping the surrounding behavior in place while avoiding the need to cd back.Source: Linters/SAST tools
62-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQuote path/variable expansions per shellcheck.
$gem5_homein the gem5.fast invocation is unquoted (lines 64-65), risking word-splitting/globbing if the path contains spaces or special characters.🔧 Proposed fix
-$gem5_home/build/RISCV/gem5.fast \ - $gem5_home/configs/example/idealkmhv3.py \ +"$gem5_home"/build/RISCV/gem5.fast \ + "$gem5_home"/configs/example/idealkmhv3.py \ --raw-cpt \ --generic-rv-cpt=$GEM5_PGO_CPT🤖 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 `@util/pgo/basic_pgo_new.sh` around lines 62 - 71, The gem5.fast invocation in basic_pgo_new.sh uses unquoted variable expansions, so update the command in the profiling block to quote the path variables referenced by the gem5_home and GEM5_PGO_CPT values. Fix the shellcheck issue by adjusting the command near the gem5.fast call so the path arguments are treated as single values even if they contain spaces or special characters.Source: Linters/SAST tools
🤖 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 `@src/cpu/valuepred/vtage.cc`:
- Around line 59-66: The VTAGE probability parameters are currently accepted
without validation, which can let invalid values silently behave as always/never
in chance(). Add constructor assertions in the VTAGE parameter initialization
path to verify every stochastic Param.Float stays within [0, 1], covering the
probabilities initialized in the constructor (including allocProbLoadL1Hit,
allocProbLoadMiss, confIncProbLowValue, confIncProbFastLoad, uIncProbFastLoad,
valueArrayUpgradeProb, shortHistoryAllocBias, and deepAllocExtraHopProb) as well
as the related probability fields in the later constructor block. Use the VTAGE
class/constructor and any helper that consumes these params as the main place to
enforce validation.
In `@util/pgo/basic_pgo_new.sh`:
- Around line 26-36: The rebuild_without_pgo function currently continues even
if cd "$gem5_home" fails, which can make the subsequent rm -rf llvm-pgo run in
the wrong directory. Update rebuild_without_pgo to hard-fail immediately when
the cd step does not succeed, before any destructive cleanup, and keep the
existing flow intact for the successful path.
---
Nitpick comments:
In `@configs/example/idealkmhv3.py`:
- Line 79: Remove the redundant explicit arb assignment where
CompositeValuePredictor is configured in the example setup, since
CVPFixedPriorityArb() is already the default for the arb parameter. Update the
call site that passes arb=CVPFixedPriorityArb() so it relies on the default
value instead, keeping the configuration cleaner without changing behavior.
In `@util/pgo/basic_pgo_new.sh`:
- Line 35: The fallback path in rebuild_without_pgo currently always returns
success, which can hide a failed PGO build or profiling step. Update
rebuild_without_pgo in basic_pgo_new.sh so it preserves and propagates the
original failure status instead of unconditionally exiting 0; make the function
return or exit with the caller’s non-zero code when it is invoked due to
profiling or profdata merge errors.
- Line 84: The script uses a plain cd .. in the PGO setup flow, which requires
later restoring the directory and is less robust. Update the basic_pgo_new.sh
logic to run that parent-directory command in a subshell instead of changing the
caller’s working directory, keeping the surrounding behavior in place while
avoiding the need to cd back.
- Around line 62-71: The gem5.fast invocation in basic_pgo_new.sh uses unquoted
variable expansions, so update the command in the profiling block to quote the
path variables referenced by the gem5_home and GEM5_PGO_CPT values. Fix the
shellcheck issue by adjusting the command near the gem5.fast call so the path
arguments are treated as single values even if they contain spaces or special
characters.
🪄 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: 8e76f790-864c-4154-9dea-76f235797f28
📒 Files selected for processing (13)
configs/example/idealkmhv3.pysrc/cpu/o3/commit.ccsrc/cpu/o3/dyn_inst.hhsrc/cpu/o3/fetch.ccsrc/cpu/o3/lsq.ccsrc/cpu/valuepred/SConscriptsrc/cpu/valuepred/ValuePredictor.pysrc/cpu/valuepred/composite_value_predictor.ccsrc/cpu/valuepred/valuepred_metadata.hhsrc/cpu/valuepred/vtage.ccsrc/cpu/valuepred/vtage.hhsrc/cpu/valuepred/vtage_metadata.hhutil/pgo/basic_pgo_new.sh
🚧 Files skipped from review as they are similar to previous changes (9)
- src/cpu/valuepred/valuepred_metadata.hh
- src/cpu/valuepred/vtage_metadata.hh
- src/cpu/o3/lsq.cc
- src/cpu/o3/dyn_inst.hh
- src/cpu/o3/fetch.cc
- src/cpu/valuepred/SConscript
- src/cpu/valuepred/vtage.hh
- src/cpu/o3/commit.cc
- src/cpu/valuepred/composite_value_predictor.cc
| allocProbLoadL1Hit(params.allocProbLoadL1Hit), | ||
| allocProbLoadMiss(params.allocProbLoadMiss), | ||
| confIncProbLowValue(params.confIncProbLowValue), | ||
| confIncProbFastLoad(params.confIncProbFastLoad), | ||
| uIncProbFastLoad(params.uIncProbFastLoad), | ||
| valueArrayUpgradeProb(params.valueArrayUpgradeProb), | ||
| shortHistoryAllocBias(params.shortHistoryAllocBias), | ||
| deepAllocExtraHopProb(params.deepAllocExtraHopProb), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate stochastic probabilities instead of silently clamping configs.
Param.Float values outside [0, 1] currently become “never” or “always” in chance(), which can hide misconfigured VTAGE training policy. Add constructor assertions for every probability param.
Suggested fix
+#include <cmath>
+
VTAGE::VTAGE(const Params ¶ms)
@@
gem5_assert(l2HitMaxCycles <= llcHitMaxCycles,
"VTAGE l2HitMaxCycles must not exceed llcHitMaxCycles");
+
+ const auto valid_probability = [](double probability) {
+ return std::isfinite(probability) &&
+ probability >= 0.0 && probability <= 1.0;
+ };
+ gem5_assert(valid_probability(allocProbLoadL1Hit),
+ "VTAGE allocProbLoadL1Hit must be in [0, 1]");
+ gem5_assert(valid_probability(allocProbLoadMiss),
+ "VTAGE allocProbLoadMiss must be in [0, 1]");
+ gem5_assert(valid_probability(confIncProbLowValue),
+ "VTAGE confIncProbLowValue must be in [0, 1]");
+ gem5_assert(valid_probability(confIncProbFastLoad),
+ "VTAGE confIncProbFastLoad must be in [0, 1]");
+ gem5_assert(valid_probability(uIncProbFastLoad),
+ "VTAGE uIncProbFastLoad must be in [0, 1]");
+ gem5_assert(valid_probability(valueArrayUpgradeProb),
+ "VTAGE valueArrayUpgradeProb must be in [0, 1]");
+ gem5_assert(valid_probability(shortHistoryAllocBias),
+ "VTAGE shortHistoryAllocBias must be in [0, 1]");
+ gem5_assert(valid_probability(deepAllocExtraHopProb),
+ "VTAGE deepAllocExtraHopProb must be in [0, 1]");Also applies to: 147-160
🤖 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/cpu/valuepred/vtage.cc` around lines 59 - 66, The VTAGE probability
parameters are currently accepted without validation, which can let invalid
values silently behave as always/never in chance(). Add constructor assertions
in the VTAGE parameter initialization path to verify every stochastic
Param.Float stays within [0, 1], covering the probabilities initialized in the
constructor (including allocProbLoadL1Hit, allocProbLoadMiss,
confIncProbLowValue, confIncProbFastLoad, uIncProbFastLoad,
valueArrayUpgradeProb, shortHistoryAllocBias, and deepAllocExtraHopProb) as well
as the related probability fields in the later constructor block. Use the VTAGE
class/constructor and any helper that consumes these params as the main place to
enforce validation.
| function rebuild_without_pgo() { | ||
| warn "$1" | ||
| cd "$gem5_home" | ||
| rm -rf llvm-pgo | ||
| CC=clang CXX=clang++ scons -c build/RISCV/gem5.fast --gold-linker | ||
| check $? | ||
| CC=clang CXX=clang++ scons build/RISCV/gem5.fast -j $build_threads --gold-linker | ||
| check $? | ||
| printf "Non-PGO build is done after PGO fallback.\n" | ||
| exit 0 | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard cd before destructive rm -rf.
If cd "$gem5_home" fails (unset/invalid path), execution continues and rm -rf llvm-pgo runs against the current (unintended) directory. Given the destructive nature of rm -rf, this should hard-fail instead of silently proceeding.
🛡️ Proposed fix
function rebuild_without_pgo() {
warn "$1"
- cd "$gem5_home"
+ cd "$gem5_home" || { echo "FAIL: cannot cd to $gem5_home"; exit 1; }
rm -rf llvm-pgo
- CC=clang CXX=clang++ scons -c build/RISCV/gem5.fast --gold-linker
+ CC=clang CXX=clang++ scons -c build/RISCV/gem5.fast --gold-linker
check $?
- CC=clang CXX=clang++ scons build/RISCV/gem5.fast -j $build_threads --gold-linker
+ CC=clang CXX=clang++ scons build/RISCV/gem5.fast -j "$build_threads" --gold-linker
check $?
printf "Non-PGO build is done after PGO fallback.\n"
exit 0
}📝 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.
| function rebuild_without_pgo() { | |
| warn "$1" | |
| cd "$gem5_home" | |
| rm -rf llvm-pgo | |
| CC=clang CXX=clang++ scons -c build/RISCV/gem5.fast --gold-linker | |
| check $? | |
| CC=clang CXX=clang++ scons build/RISCV/gem5.fast -j $build_threads --gold-linker | |
| check $? | |
| printf "Non-PGO build is done after PGO fallback.\n" | |
| exit 0 | |
| } | |
| function rebuild_without_pgo() { | |
| warn "$1" | |
| cd "$gem5_home" || { echo "FAIL: cannot cd to $gem5_home"; exit 1; } | |
| rm -rf llvm-pgo | |
| CC=clang CXX=clang++ scons -c build/RISCV/gem5.fast --gold-linker | |
| check $? | |
| CC=clang CXX=clang++ scons build/RISCV/gem5.fast -j "$build_threads" --gold-linker | |
| check $? | |
| printf "Non-PGO build is done after PGO fallback.\n" | |
| exit 0 | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 28-28: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[info] 32-32: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@util/pgo/basic_pgo_new.sh` around lines 26 - 36, The rebuild_without_pgo
function currently continues even if cd "$gem5_home" fails, which can make the
subsequent rm -rf llvm-pgo run in the wrong directory. Update
rebuild_without_pgo to hard-fail immediately when the cd step does not succeed,
before any destructive cleanup, and keep the existing flow intact for the
successful path.
Source: Linters/SAST tools
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
VTAGE is largely based on the open-source implementation of the 1st-place Championship Value Prediction (CVP-1) submission. The version in this codebase is adapted and modified for our environment. It is not intended to be a bit-exact copy of the original code. For detailed background and reference material: + Paper: https://microarch.org/cvp1/papers/Seznec.pdf + Open-source implementation: https://www.microarch.org/cvp1/code/Seznec.tar.gz + Official website: https://www.microarch.org/cvp1/ Change-Id: If6bbd218061b2836690390ca1e3f06788df30100
1 participant
The idea of using VTAGE (or E-VTAGE) for value prediction was first proposed in the 1st-place Championship Value Prediction (CVP-1) submission.
VTAGE implementation in this PR is adapted from the CVP1 Open-source implementation and modified for our environment. It is not intended to be a bit-exact copy of the original code.
The parameters in the current implementation have not been finely tuned; this task is left to interested parties for further optimization.
The VTAGE algorithm was ported to provide a state-of-the-art baseline for research into value prediction — specifically, the EVES predictor (comprising EStride and EVTAGE) proposed in the CVP1.
For detailed background and reference material:
Change-Id: If6bbd218061b2836690390ca1e3f06788df30100
Summary by CodeRabbit