Repository navigation
cpu-o3: Replay conflicting SMT load-reserved operations - #1160
jensen-yan wants to merge 2 commits into
Conversation
Change-Id: Ia9625dd3502c72681b93919bf7e7489a4fc4a2ef
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe out-of-order CPU now replays executed load-reserved instructions when a conflicting store from another thread becomes visible. The reorder buffer reports when an uncommitted load-reserved instruction blocks speculative store publication, and Commit records those cycles. ChangesLoad-reserved ordering
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change addresses conflicting SMT load-reserved operations, with no established issue requiring a fix before merge. The reported checkpoint limitation and incomplete benchmark reruns remain validation context, not evidence of a regression introduced here. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39b14a6ab6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
The replay predicate also covers ARM and MIPS linked loads and must be gated or explicitly supported before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds O3 SMT handling to replay executed load-reserved operations after conflicting stores become visible.
Changes:
- Replays conflicting executed LRs through the existing fault path.
- Adds LR replay and ignored-load statistics.
- Reuses bounded LSQ visibility scans.
File summaries
| File | Summary |
|---|---|
src/cpu/o3/lsq_unit.hh |
Declares new LSQ statistics. |
src/cpu/o3/lsq_unit.cc |
Registers statistics and replays conflicting executed LLSC loads. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
jensen-yan
added
the
Ready to Merge
Change-Id: I4dbb3d51db0c8865d627fdd10e253e1be492fc04
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Motivation
The SPEC2017 SMT run in Actions run 34924968908 reproducibly failed in
mcf_rate_refrate_0at tick 678519468. CPU 0 committedlr.w a5, (a2)at PC0xffffffff80152788and physical address0x47e951534with value 7, while the reference and current shared-memory value were 8.When a store becomes visible,
notifyOtherThreadsStoreVisible()already checks byte-overlapping loads in the other SMT contexts. The existing path:That gap allows an LR result from before another context's overlapping visible store to remain speculative and later reach commit.
Approach
When
checkLocalStoreVisible()finds an overlapping executed LR with no existing fault, this change assignsReExecand marks its active request faulted. The LR is therefore squashed and retried through the normal commit-time fault path.Ordinary executed loads keep their previous behavior: observing the older value may be legal, so they are not replayed unconditionally.
Assigning
ReExecalone is not sufficient. Commit runs before IEW in a CPU tick, while IEW consumes the previousdoneMemSeqNumonly after LSQ writeback processing. A stale speculativedoneMemSeqNumcan therefore authorize a younger store before the LR squash reaches IEW. Once that store enters the store buffer, the existingisSquashed()check is too late to revoke it.To close that race at its source,
ROB::getHeadGroupLastDoneSeq()now stops at every uncommittedisLoadReserved()instruction. Speculative store-publication authorization can no longer cross an LR whose result may still receiveReExec; the normal committed path can release younger stores after the LR commits. The replay predicate is also narrowed from the genericRequest::LLSCflag toDynInst::isLoadReserved().This is intentionally a conservative behavioral model:
Three statistics make the modeled boundary observable:
smtVisibleStoreLrReexec: executed LRs replayed by another thread's visible store;smtVisibleStoreExecutedLoadIgnored: executed ordinary loads deliberately kept.commit.loadReservedStorePublishBarrierCycles: cycles where an uncommitted LR bounds speculative store publication.There are no new parameters or configuration changes.
Validation
scons build/RISCV/gem5.opt --gold-linker -j64passes.python3 util/style.py -m src/cpu/o3/commit.cc src/cpu/o3/commit.hh src/cpu/o3/rob.cc src/cpu/o3/rob.hh src/cpu/o3/lsq_unit.cc src/cpu/o3/lsq_unit.hhpasses.git diff --checkpasses.commitWidth=1completed normally with 239,343 committed instructions, 214,379 cycles, and 3loadReservedStorePublishBarrierCycles, directly confirming that the new authorization barrier is exercised.mcf_rate_refrate_0checkpoint with the original multi-hart NEMU reference, a 7M any-thread-limit run on reproduction-compatible revision4c63267dceplus the original LR replay change (before the store-publication barrier commit) completed normally:The exact original sequence number/tick is not expected to remain stable because an earlier LR replay changes subsequent SMT scheduling. The historical checkpoint evidence is that the model-driven replay path fires, the run passes the original failure window, and difftest remains clean until the requested limit.
Validation limitations
--disable-difftest; they validate forward progress and the replay/barrier control paths, not reference-model equivalence.doneMemSeqNum/canWBwindow was not deterministically reproduced under the default SMT configuration:CROB_instPerGroup=2, and the RISC-V LR expands into two micro-ops, placing the following store outside the same head group. The fix instead follows the confirmed CPU/commit/IEW source ordering and closes authorization at the source.csrrw tp, mscratch, tp, PC0x80100508, differingmepc).The full SPEC2017 SMT slice and performance CI have not been rerun yet.