Skip to content

mem: Fix a RAR/RAWQueue bug.#722

Merged
Ergou-ren merged 1 commit intoxs-devfrom
fix-rar-raw
Jan 22, 2026
Merged

mem: Fix a RAR/RAWQueue bug.#722
Ergou-ren merged 1 commit intoxs-devfrom
fix-rar-raw

Conversation

@Ergou-ren
Copy link
Copy Markdown
Collaborator

@Ergou-ren Ergou-ren commented Jan 20, 2026

The current load/storeCompletedIdx ignores certain situations, which may cause some instructions before Idx to not have been committed.

Change-Id: I7f79ad2ea0bc305f9d389e4372b45a453bf59278

Summary by CodeRabbit

  • Refactor
    • Refined completion tracking logic in queue management systems to improve state consistency and boundary handling for more reliable operation sequencing.

✏️ Tip: You can customize this high-level summary in your review settings.

The current load/storeCompletedIdx ignores certain situations,
which may cause some instructions before Idx to not have been committed.

Change-Id: I7f79ad2ea0bc305f9d389e4372b45a453bf59278
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 20, 2026

📝 Walkthrough

Walkthrough

The LSQ unit's completion index tracking mechanism was refactored to use a sentinel convention (head()-1) for initialized indices, enforce strict boundary constraints, replace per-iteration progression with guarded advancement loops, and introduce a unified rewindCompletedIdx mechanism to handle squash operations consistently.

Changes

Cohort / File(s) Summary
LSQ Unit Completion Index Tracking
src/cpu/o3/lsq_unit.cc
Refactored completion index initialization to use head()-1 as sentinel value; added boundary enforcement in updateCompletedIdx; replaced direct progression logic with guarded loops that only advance when queues are non-empty and entries are valid/ready; introduced unified rewindCompletedIdx lambda to handle squash-time boundary restoration for both load and store queues

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Suggested reviewers

  • happy-lx
  • jensen-yan
  • tastynoob

Poem

🐰 Queues once tangled, now aligned,
Sentinels guard each weary line,
Boundaries held, no squash in sight,
Completion tracking—shining bright!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: fixing a bug in RAR/RAWQueue load/storeCompletedIdx handling, which is the core focus of the modifications to lsq_unit.cc.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@src/cpu/o3/lsq_unit.cc`:
- Around line 2904-2917: The loop that advances loadCompletedIdx can call
loadQueue.getIterator(loadCompletedIdx + 1) when loadCompletedIdx equals the
queue tail, producing an invalid iterator; before calling getIterator(check),
add a bounds check using loadQueue.tail() or loadQueue.size() to ensure
loadCompletedIdx + 1 is within the queue range (e.g., if (loadCompletedIdx + 1
<= loadQueue.tail()) ) and only then obtain loadIt and inspect
loadIt->valid()/instruction()/isExecuted(); keep the rest of the logic
(loadCompletionWidth, DPRINTF, incrementing loadCompletedIdx) unchanged.
- Around line 2920-2932: The loop advances storeCompletedIdx by calling
storeQueue.getIterator(storeCompletedIdx + 1) without checking bounds; when
storeCompletedIdx == storeQueue.tail() that +1 access is out of range. Fix by
adding a bounds check before calling getIterator (or change the loop condition)
so you only attempt getIterator(storeCompletedIdx + 1) when storeCompletedIdx !=
storeQueue.tail(); i.e., ensure the loop over storeCompletionWidth also verifies
storeCompletedIdx != storeQueue.tail() and break if it is, before using
storeQueue.getIterator, keeping use of storeQueue, storeCompletedIdx,
storeCompletionWidth, storeQueue.getIterator, addrReady(), and canWB() intact.

@github-actions
Copy link
Copy Markdown

🚀 Coremark Smoke Test Results

Branch IPC Change
Base (xs-dev) 2.1739 -
This PR 2.1739 ➡️ 0.0000 (0.00%)

✅ Difftest smoke test passed!

@Ergou-ren Ergou-ren merged commit e0a2739 into xs-dev Jan 22, 2026
2 checks passed
@Ergou-ren Ergou-ren deleted the fix-rar-raw branch January 22, 2026 06:53
jueshiwenli pushed a commit that referenced this pull request Jan 24, 2026
The current load/storeCompletedIdx ignores certain situations,
which may cause some instructions before Idx to not have been committed.

Change-Id: I7f79ad2ea0bc305f9d389e4372b45a453bf59278
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants