Mgsc2 align#686
Conversation
- Updated calculatePercsum and calculateScaledPercsum documentation for clarity. - Enhanced MgscStats to include new statistics for weight scale sensitivity and prediction correctness.
Change-Id: I5992adb066d5c5c75bd16141564ac55264aa7e0a
Change-Id: Ieaef1b35cd968cb97679b982eab6db54b0b74b4c
- Enhanced the scaling formula in the calculateScaledPercsum function to implement a 4-level granularity for weight adjustments. - The new scaling logic provides more nuanced control over the importance of percsum based on weight ranges. Change-Id: I45dda73bec5d79d3521fec3ce7244c8e3c52dcab
Change-Id: Ib26755301387cbf1ce4b12bb26e988d43a49eea8
📝 WalkthroughWalkthroughThe PR adds comprehensive diagnostic infrastructure for the MGSC predictor including detailed technical documentation, new statistics counters for tracking per-table performance and threshold dynamics, test control switches for selectively enabling/disabling SC tables, and a trace structure for recording MGSC/TAGE prediction data. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 Ruff (0.14.10)src/cpu/pred/BranchPredictor.py1150-1150: (F405) 1151-1151: (F405) 1152-1152: (F405) 1153-1153: (F405) 1154-1154: (F405) 1155-1155: (F405) 1156-1156: (F405) 1157-1157: (F405) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cpu/pred/btb/btb_mgsc.hh (1)
505-505: Fix the incorrect header guard comment.The comment references
TAGEbut this is the MGSC header file. This appears to be a copy-paste artifact.🔎 Proposed fix
-#endif // __CPU_PRED_BTB_TAGE_HH__ +#endif // __CPU_PRED_BTB_MGSC_HH__
🧹 Nitpick comments (2)
src/cpu/pred/btb/btb_mgsc.cc (1)
1170-1171: Minor formatting inconsistency.Line 1170 has a space after the comma in the description string, but line 1171 does not. This is a nitpick but maintaining consistency improves readability.
🔎 Proposed fix
- ADD_STAT(scPredCorrectTageWrong, statistics::units::Count::get(),"number of sc prediction correct and tage wrong"), - ADD_STAT(scPredWrongTageCorrect, statistics::units::Count::get(),"number of sc prediction wrong and tage correct"), + ADD_STAT(scPredCorrectTageWrong, statistics::units::Count::get(), "number of sc prediction correct and tage wrong"), + ADD_STAT(scPredWrongTageCorrect, statistics::units::Count::get(), "number of sc prediction wrong and tage correct"),src/cpu/pred/btb/btb_mgsc.hh (1)
97-132: Consider refactoring the large constructor signature.The
MgscPredictionconstructor now takes 25+ parameters, which is unwieldy and error-prone. Consider grouping related fields into nested structs (e.g.,PercsumData,WeightScaleData,IndexData) or using a builder pattern in a future refactor.Example structure:
struct PercsumData { int bw, l, i, g, p, bias; }; struct WeightScaleData { bool bw, l, i, g, p, bias; }; // Then: MgscPrediction(..., PercsumData percsums, WeightScaleData weightDiffs);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
configs/example/kmhv3.pydocs/Gem5_Docs/frontend/mgsc_notes.mdsrc/cpu/pred/btb/btb_mgsc.ccsrc/cpu/pred/btb/btb_mgsc.hh
🧰 Additional context used
🧬 Code graph analysis (1)
src/cpu/pred/btb/btb_mgsc.hh (2)
src/cpu/pred/btb/btb_tage.hh (2)
taken(282-282)pred(383-383)src/cpu/pred/btb/btb_ittage.hh (1)
taken(179-179)
🪛 LanguageTool
docs/Gem5_Docs/frontend/mgsc_notes.md
[uncategorized] ~14-~14: 您的意思是“少"于"”吗？
Context: ...好。 这些表使用折叠历史压缩到较短索引位宽，降低端口和存储压力，同时减少与 TAGE 的直接结构耦合。 - 决策与门控：各表计数器 percsum 做线性...
(YU7_YU8)
[uncategorized] ~46-~46: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：丰富"地"可
Context: ...E 置信度（高置信错时更快抬阈，低置信错时慢抬），减少误覆盖。 4) 更丰富的可观测性：按表/置信度输出热分支热点，或导出 per-PC 纠错热度。 ## ...
(wb4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quick Build, Unit Tests & Smoke Test
- GitHub Check: perf_test / XS-GEM5 - Run performance test (spec06-0.3c)
🔇 Additional comments (9)
configs/example/kmhv3.py (1)
108-108: LGTM!Enabling the MGSC predictor aligns with the PR objective for SC testing. The placement within the
DecoupledBPUWithBTBconfiguration block is appropriate.docs/Gem5_Docs/frontend/mgsc_notes.md (1)
1-54: Good documentation for MGSC integration.The documentation provides a comprehensive overview of the MGSC predictor design, data structures, statistics, and test cases. The interpretation tips for statistics counters will be particularly helpful for debugging and optimization.
src/cpu/pred/btb/btb_mgsc.cc (4)
191-196: Disabled weight scaling for testing - ensure this is intentional.The weight scaling logic is completely bypassed by returning
percsumdirectly. This effectively disables a core feature of the MGSC predictor. The comment indicates this is for testing purposes, but this should not be merged to production without re-enabling or adding a configurable flag.Consider using a parameter to control this behavior instead of hardcoding:
🔎 Suggested approach
// Add a parameter like p.enableWeightScaling in the constructor // Then use: int BTBMGSC::calculateScaledPercsum(int weight, int percsum) { if (!enableWeightScaling) return percsum; // Original scaling logic here }
543-628: Good centralization of statistics collection.The
recordPredictionStatsfunction cleanly consolidates all MGSC-related statistics recording. The use of lambdas forrecordPercsumandrecordConfOutcomereduces code duplication effectively.
652-665: LGTM!The threshold statistics correctly track when thresholds are being adjusted. The pairing of PC and global threshold stats reflects the actual update behavior at lines 698-701.
332-336: LGTM!The
MgscPredictionconstructor call correctly passes the new TAGE confidence fields along with all other required parameters in the proper order.src/cpu/pred/btb/btb_mgsc.hh (3)
42-44: LGTM!The new TAGE confidence fields (
tage_conf_high,tage_conf_mid,tage_conf_low) are appropriately added to capture the confidence level at prediction time for later use in statistics and update logic.
271-272: LGTM!The
recordPredictionStatsdeclaration is correctly placed in the private section and matches the implementation signature.
412-449: Good statistics instrumentation.The new statistics fields provide comprehensive observability into MGSC behavior:
- Weight scale sensitivity per table type
- Raw percsum correctness tracking
- Threshold adjustment monitoring
- SC usage breakdown by TAGE confidence level
This aligns well with the documentation in
mgsc_notes.md.
Change-Id: I69ef16d0b2d973de03e8ccd381ff359bc640d5f1
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/cpu/pred/btb/btb_mgsc.hh (1)
97-103: Consider using const references for vector parameters.The constructor takes
std::vectorparameters by value, which creates copies. For better efficiency, consider passing byconst&:- MgscPrediction(Addr btb_pc, int total_sum, bool use_mgsc, bool taken, bool taken_before_sc, - bool tage_conf_high, bool tage_conf_mid, bool tage_conf_low, int16_t total_thres, - std::vector<unsigned> bwIndex, std::vector<unsigned> lIndex, std::vector<unsigned> iIndex, - std::vector<unsigned> gIndex, std::vector<unsigned> pIndex, std::vector<unsigned> biasIndex, + MgscPrediction(Addr btb_pc, int total_sum, bool use_mgsc, bool taken, bool taken_before_sc, + bool tage_conf_high, bool tage_conf_mid, bool tage_conf_low, int16_t total_thres, + const std::vector<unsigned>& bwIndex, const std::vector<unsigned>& lIndex, const std::vector<unsigned>& iIndex, + const std::vector<unsigned>& gIndex, const std::vector<unsigned>& pIndex, const std::vector<unsigned>& biasIndex,src/cpu/pred/btb/btb_mgsc.cc (2)
198-203: Weight scaling is disabled for testing - consider adding a configuration flag.The
calculateScaledPercsumfunction currently ignores theweightparameter and just returnspercsum. This disables a significant feature of the SC predictor.If this is temporary for testing, consider:
- Adding a TODO comment with ticket reference
- Using a configuration flag (similar to the new table enable flags) to control this behavior
int BTBMGSC::calculateScaledPercsum(int weight, int percsum) { - return percsum; // disable weight scaling for test + // TODO: Re-enable weight scaling after testing + // return (weight >= 0) ? percsum * (1 + weight) : percsum / (1 - weight); + return percsum; }Would you like me to propose a configuration flag to toggle weight scaling on/off?
278-300: Minor optimization: Skip weight lookup when table is disabled.When a table is disabled, its weight is still looked up but never used (since
scaled_percsumwill be 0). Consider:- int bw_percsum = enableBwTable ? calculatePercsum(bwTable, bwIndex, bwTableNum, btb_entry.pc) : 0; - int bw_weight = findWeight(bwWeightTable, btb_entry.pc); - int bw_scaled_percsum = calculateScaledPercsum(bw_weight, bw_percsum); + int bw_percsum = 0, bw_weight = 0, bw_scaled_percsum = 0; + if (enableBwTable) { + bw_percsum = calculatePercsum(bwTable, bwIndex, bwTableNum, btb_entry.pc); + bw_weight = findWeight(bwWeightTable, btb_entry.pc); + bw_scaled_percsum = calculateScaledPercsum(bw_weight, bw_percsum); + }This would avoid unnecessary memory accesses for disabled tables.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
configs/example/kmhv3.pysrc/cpu/pred/BranchPredictor.pysrc/cpu/pred/btb/btb_mgsc.ccsrc/cpu/pred/btb/btb_mgsc.hh
🚧 Files skipped from review as they are similar to previous changes (1)
- configs/example/kmhv3.py
🧰 Additional context used
🧬 Code graph analysis (2)
src/cpu/pred/btb/btb_mgsc.hh (3)
src/cpu/pred/btb/btb_tage.hh (2)
taken(282-282)pred(383-383)src/cpu/pred/btb/stream_struct.hh (2)
taken(640-668)taken(704-704)src/cpu/pred/btb/btb_ittage.hh (1)
taken(179-179)
src/cpu/pred/btb/btb_mgsc.cc (1)
src/cpu/pred/btb/btb_mgsc.hh (7)
entry(169-169)entry(269-270)weight(198-198)total_sum(208-208)MgscPrediction(66-95)MgscPrediction(97-132)pred(271-272)
🪛 Ruff (0.14.10)
src/cpu/pred/BranchPredictor.py
1150-1150: Param may be undefined, or defined from star imports
(F405)
1151-1151: Param may be undefined, or defined from star imports
(F405)
1152-1152: Param may be undefined, or defined from star imports
(F405)
1153-1153: Param may be undefined, or defined from star imports
(F405)
1154-1154: Param may be undefined, or defined from star imports
(F405)
1155-1155: Param may be undefined, or defined from star imports
(F405)
1156-1156: Param may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: perf_test / XS-GEM5 - Run performance test (spec06-0.3c)
🔇 Additional comments (11)
src/cpu/pred/btb/btb_mgsc.hh (4)
42-44: LGTM: TAGE confidence fields added to MgscPrediction.The new
tage_conf_high,tage_conf_mid, andtage_conf_lowfields are properly initialized in both constructors, enabling confidence-based SC usage decisions.Also applies to: 72-74, 109-111
329-336: LGTM: Per-table enable flags provide good configurability.These configuration flags allow fine-grained control over individual SC tables, which is useful for testing and debugging the contribution of each table to prediction accuracy.
421-458: LGTM: Comprehensive statistics for SC predictor analysis.The new statistics provide excellent visibility into:
- Per-table weight scaling decisions
- Per-table percsum correctness
- Threshold adaptation behavior
- SC usage patterns under different TAGE confidence levels
This will enable detailed analysis of the predictor's behavior.
271-272: LGTM: Centralized statistics recording method.The
recordPredictionStatsmethod centralizes statistics recording logic, improving code organization.src/cpu/pred/btb/btb_mgsc.cc (6)
55-61: LGTM: New configuration flags properly initialized.The new per-table enable flags and
forceUseSCare correctly initialized from the Params object.
552-637: LGTM: Well-structured statistics recording.The
recordPredictionStatsmethod is well-organized with:
- Clear separation of concerns using lambdas (
recordPercsum,recordConfOutcome)- Comprehensive coverage of all statistics dimensions
- Efficient inline recording
Good use of lambdas to avoid code duplication.
661-674: LGTM: Statistics recording integrated into update path.Statistics are correctly recorded:
recordPredictionStatsis called for all predictions- Threshold increment/decrement stats are only recorded when tables are actually updated
1180-1215: LGTM: New statistics properly registered.All new statistics are correctly initialized with descriptive names and appropriate units. The grouping by category (weight scaling, percsum, threshold, SC usage by confidence) is logical.
159-177: LGTM: Improved documentation for percsum calculation.The added comments clearly explain:
- Counter value range based on width
- The transformation to odd numbers (
2*counter + 1)- Sign semantics (positive = taken bias, negative = not-taken bias)
This improves code maintainability.
312-327: The confidence level logic is exhaustive and always assigns at least one flag.The three confidence flags are mutually exclusive and exhaustively cover all cases:
- When
pred.mainInfo.foundis false,tage_pred_conf_lowis always true.- When found and the counter is saturated (abs value = 7),
tage_pred_conf_highis true.- When found and the counter is mid-range (abs value = 3 or 5),
tage_pred_conf_midis true.- When found and the counter is near zero (abs value = 1),
tage_pred_conf_lowis true.The code in btb_mgsc.cc lines 312-327 is correct and will always find at least one true condition in the if-else chain.
src/cpu/pred/BranchPredictor.py (1)
1149-1157: LGTM: New configuration parameters properly defined.The new parameters match the C++ declarations:
forceUseSCdefaults toFalse(SC usage based on confidence thresholds)- All table enable flags default to
True(all tables enabled by default)The Ruff warnings about
Parambeing undefined are false positives —Paramis imported viafrom m5.params import *on line 29, which is the standard gem5 configuration pattern.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
Change-Id: Ia279ea56c55b79261163bee4400befe6014e8c12
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
This reverts commit 2cfb00c.
Change-Id: I6ecce2060d5a9dc85312ff87188ee8804a694f2a
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cpu/pred/btb/btb_mgsc.hh (1)
97-103: Constructor signature is getting large.The MgscPrediction constructor now accepts 21 parameters, which makes it difficult to maintain and error-prone to call. Consider using a builder pattern or designated initializers (C++20) for better readability and maintainability.
Example builder pattern approach
struct MgscPredictionBuilder { MgscPrediction pred; MgscPredictionBuilder& setBtbPc(Addr pc) { pred.btb_pc = pc; return *this; } MgscPredictionBuilder& setTotalSum(int sum) { pred.total_sum = sum; return *this; } // ... other setters ... MgscPrediction build() { return pred; } };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
configs/example/kmhv3.pysrc/cpu/pred/BranchPredictor.pysrc/cpu/pred/btb/btb_mgsc.ccsrc/cpu/pred/btb/btb_mgsc.hh
🧰 Additional context used
🧬 Code graph analysis (1)
src/cpu/pred/btb/btb_mgsc.hh (2)
src/cpu/pred/btb/btb_tage.hh (2)
taken(282-282)pred(383-383)src/cpu/pred/btb/btb_ittage.hh (1)
taken(179-179)
🪛 Ruff (0.14.10)
src/cpu/pred/BranchPredictor.py
1150-1150: Param may be undefined, or defined from star imports
(F405)
1151-1151: Param may be undefined, or defined from star imports
(F405)
1152-1152: Param may be undefined, or defined from star imports
(F405)
1153-1153: Param may be undefined, or defined from star imports
(F405)
1154-1154: Param may be undefined, or defined from star imports
(F405)
1155-1155: Param may be undefined, or defined from star imports
(F405)
1156-1156: Param may be undefined, or defined from star imports
(F405)
1157-1157: Param may be undefined, or defined from star imports
(F405)
🔇 Additional comments (13)
configs/example/kmhv3.py (2)
108-111: Verify the performance impact of forcing SC usage.Enabling MGSC with
forceUseSC = Truefundamentally changes branch prediction behavior by always using the statistical corrector. The performance robot reports show significant variance across commits, including a -72% regression in commit 2cfb00c that was later reverted.Confirm that this configuration matches the intended RTL behavior and that the performance characteristics are acceptable for your testing objectives.
113-120: LGTM: RTL alignment configuration matches stated intent.The selective table enablement (PTable and BiasTable only, with all other tables disabled) aligns with the PR objective to match RTL behavior. The configuration is internally consistent with the inline comment.
src/cpu/pred/btb/btb_mgsc.hh (3)
42-44: LGTM! TAGE confidence fields properly added.The three TAGE confidence levels (high, mid, low) are consistently added to the MgscPrediction struct across default initialization, constructor parameters, and member initialization. This allows the SC predictor to adapt its behavior based on TAGE confidence.
Also applies to: 72-74, 109-111
422-459: Comprehensive statistics added for MGSC analysis.The expanded MgscStats provides excellent observability into:
- Per-table weight scale sensitivity
- Raw percsum correctness for each table
- Threshold update patterns
- SC usage patterns under different TAGE confidence levels
This will be valuable for understanding predictor behavior and tuning.
329-337: Enable flag defaults and safety checks are properly implemented.All enable flags default to
Truein BranchPredictor.py (exceptforceUseSCwhich defaults toFalse), matching the full production MGSC configuration. The implementation safely handles disabled tables using ternary guards throughout btb_mgsc.cc (e.g.,enableBwTable ? calculatePercsum(...) : 0), preventing both uninitialized memory access and divide-by-zero issues. Each flag is documented with clear intent descriptions in BranchPredictor.py.src/cpu/pred/btb/btb_mgsc.cc (7)
55-62: LGTM! Enable flags properly initialized from parameters.The constructor correctly initializes all enable flags from the provided parameters, allowing runtime configuration of MGSC table usage.
162-169: Good documentation of percsum calculation.The updated documentation clearly explains the counter range transformation to odd numbers (2*counter + 1), which avoids zero and provides a clear bias signal. The formula correctly maps the counter range [-32, 31] to percsum range [-63, 63] for 6-bit counters.
Also applies to: 178-178
279-279: Conditional table usage properly implemented.The percsum calculations correctly check the corresponding enable flags before computing values, returning 0 when disabled. This provides clean control over which tables contribute to the final prediction.
Also applies to: 283-283, 287-287, 291-291, 295-295, 299-299
553-638: Well-structured statistics collection method.The
recordPredictionStatsmethod provides centralized, comprehensive statistics tracking:
- SC vs TAGE outcome tracking
- Per-table percsum correctness using a helper lambda
- Weight scale difference tracking
- Confidence-based SC usage patterns
The use of lambdas (
recordPercsumandrecordConfOutcome) is elegant and reduces code duplication.
668-675: Statistics tracking for threshold updates.The code correctly increments/decrements threshold statistics based on prediction correctness. However, note that these statistics are always incremented regardless of the
enablePCThresholdflag value, while the actual PC threshold update (lines 707-710) is conditional. This is correct behavior since global thresholds are always updated, but the asymmetry is worth noting.
707-710: PC threshold updates properly gated by enable flag.The PC-indexed threshold table is only updated when
enablePCThresholdis true, providing correct conditional behavior. The global threshold (line 713) is always updated, which is appropriate.
313-328: Confidence-based threshold logic verified as correct.The tiered threshold approach is sound: saturated TAGE predictions (high confidence) use the most lenient threshold (total_thres/2), unsaturated predictions use moderate thresholds, and unpredicted/low-confidence cases use stricter thresholds. The three confidence levels are mutually exclusive by design based on counter saturation state. Fallthrough behavior when no confidence level matches (which shouldn't occur) is intentional.
src/cpu/pred/BranchPredictor.py (1)
1149-1157: MGSC control parameters properly defined.The new parameters provide runtime configurability for MGSC behavior:
forceUseSC: Defaults to False, allowing normal TAGE confidence-based switching- Table enable flags: Default to True, enabling all tables by default
enablePCThreshold: Defaults to True, enabling PC-indexed thresholdsDefault values appear reasonable for production use, with all features enabled except forceUseSC.
Note: The static analysis hints about undefined
Paramare false positives—Paramis correctly imported viafrom m5.params import *on line 29.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
Change-Id: I2b8451ff6ec9abbda07d71a32a942fb116f0abd9
global + bias + path Change-Id: I58b509b2361a76b681a3862bb3d9bcc140d5ac52
IMLI + global + bias + path Change-Id: I5a9c8470901455db75597a3d0aa07204a008a291
backward + IMLI + global + bias + path Change-Id: I7aabaafa68660f82a75a19e002f0b44376751c79
local + backward + IMLI + global + bias + path Change-Id: I6ec878fc09049b5cab9a840ed7e4b568f39168fd
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cpu/pred/btb/stream_struct.hh (1)
801-836: LGTM! MgscTrace structure properly extends Record for MGSC/TAGE tracing.The structure correctly captures MGSC prediction data including TAGE confidence levels, per-table percsums, and final SC decision. The signed int64_t percsum values are intentionally cast to uint64_t for storage (lines 822-830), preserving the bit pattern for later analysis.
The
set()method has 16 parameters, which can be difficult to maintain. Consider grouping related fields into nested structures if this interface becomes unwieldy in practice.💡 Optional refactor: group related parameters
For example, you could group TAGE confidence fields:
struct TageConfidence { uint64_t high; uint64_t mid; uint64_t low; }; struct PercsumValues { int64_t bw, l, i, g, p, bias; }; void set(uint64_t branchPC, uint64_t tagePred, const TageConfidence& conf, const PercsumValues& percsums, int64_t totalSum, int64_t totalThres, uint64_t useSc, uint64_t scPred, uint64_t actualTaken)However, the current flat structure is acceptable and matches the existing pattern in this codebase.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
configs/example/kmhv3.pysrc/cpu/pred/btb/btb_mgsc.ccsrc/cpu/pred/btb/stream_struct.hh
🚧 Files skipped from review as they are similar to previous changes (1)
- configs/example/kmhv3.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/cpu/pred/btb/stream_struct.hh (1)
src/sim/clocked_object.hh (1)
curTick(222-222)
🔇 Additional comments (8)
src/cpu/pred/btb/btb_mgsc.cc (8)
55-62: LGTM! Proper initialization of SC control flags.The new configuration parameters (forceUseSC, per-table enable flags, enablePCThreshold) are properly initialized from the Params object, providing fine-grained control over MGSC/SC behavior.
149-172: LGTM! Trace initialization properly defines MGSC trace schema.The
setTrace()method correctly initializes the MGSC trace with 16 fields that align with theMgscTrace::set()method signature. The implementation is properly guarded with#ifndef UNIT_TESTand checksenableDBbefore initialization.
188-207: LGTM! Percsum calculation correctly transforms counters to odd numbers.The formula
percsum += (2 * entry + 1)(line 204) properly transforms counter values to odd numbers, avoiding zero contributions. The updated documentation clearly explains the counter range and transformation logic.
305-354: LGTM! Conditional table enabling and SC usage logic are correctly implemented.The per-table percsum calculations properly respect the enable flags (lines 305, 309, 313, 317, 321, 325), setting contributions to 0 when disabled. The SC usage decision logic (lines 339-354) correctly implements confidence-based thresholding:
forceUseSCbypasses all checks- High TAGE confidence requires
|total_sum| > total_thres/2- Mid confidence requires
|total_sum| > total_thres/4- Low confidence requires
|total_sum| > total_thres/8This ensures SC only overrides TAGE when it has sufficient confidence, with higher bars for overriding high-confidence TAGE predictions.
579-664: LGTM! Centralized statistics recording with good code structure.The new
recordPredictionStats()method consolidates statistics collection, improving maintainability. The implementation correctly:
- Tracks SC vs TAGE outcomes (lines 588-601)
- Records per-table percsum correctness using a lambda helper (lines 604-639)
- Tracks weight scaling criticality for each table
- Records SC usage outcomes bucketed by TAGE confidence levels (lines 642-663)
The use of lambda functions reduces code duplication while maintaining clarity.
688-702: LGTM! Statistics recording and trace writing properly integrated.The
updateSinglePredictormethod correctly callsrecordPredictionStats()(line 688) and writes trace records (lines 690-702). The trace writing is properly guarded with#ifndef UNIT_TESTandenableDBchecks, and all required fields are passed toMgscTrace::set().
708-750: LGTM! Threshold update logic correctly adapts SC usage.The threshold adjustment logic (lines 708-715) is correct:
- Increments when
sc_pred_taken != actual_taken(raise the bar after SC errors)- Decrements when SC is correct but confidence is low (lower the bar to use SC more often)
Statistics tracking (pcThresholdInc/Dec, globalThresholdInc/Dec) provides visibility into threshold adaptation. The PC-indexed threshold update (lines 747-750) properly respects the
enablePCThresholdflag.
1225-1258: LGTM! Comprehensive MGSC statistics for observability.The new statistics counters provide excellent visibility into MGSC behavior:
- Per-table weight scaling criticality (lines 1225-1230)
- Per-table percsum correctness (lines 1232-1243)
- Threshold adaptation tracking (lines 1245-1248)
- SC usage outcomes bucketed by TAGE confidence (lines 1250-1258)
The statistics align with the
recordPredictionStats()method and follow gem5 conventions. This comprehensive instrumentation will be valuable for MGSC performance analysis and tuning.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
1 similar comment
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
Change-Id: I07ece3caa9617ff7c1aa094d00ef9710d476b41e
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
Change-Id: I102752018085baeae8aaba087b361758e6af2ab4
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
3 participants
for sc test
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.