Xs vector 2605021#879
Conversation
- Add VectorSplitQ to prepare for the secondary splitting of vector memory access instructions. Change-Id: I17cb39915e8198de27144ed2ea1078eb6c559b0d
Change-Id: Ifb05c47c012236d68b073d3b9fcaaf7c518d6b3e
📝 WalkthroughWalkthroughAdds a vector-ready instruction path to IssueQue: new queues and seq tracking, constructor event wiring to schedule releases, routing of ready vector memory ops into the vector queue, timed transfer into a delayed-ready queue, pre-issuance with gating/limits, and commit/squash cleanup. ChangesVector-Ready Instruction Queue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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)
⚔️ Resolve merge conflicts
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/cpu/o3/issue_queue.ccsrc/cpu/o3/issue_queue.cc:1:10: fatal error: 'cpu/o3/issue_queue.hh' file not found ... [truncated 1089 characters] ... g/install/lib/clang/18/include" 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 |
|
🚀 RVV Performance test triggered: spec06int-rvv-0.8c |
There was a problem hiding this comment.
Pull request overview
This PR adds a separate ready queue for vector memory instructions in the O3 issue queue so they can be prioritized after replay traffic and before the normal FU issue stream.
Changes:
- Adds
vectorReadyQtracking and duplicate suppression by sequence number. - Routes ready vector memory instructions into the new queue.
- Issues vector memory instructions from the new queue before normal
toFuinstructions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/cpu/o3/issue_queue.hh |
Adds vector-ready queue state and accessors. |
src/cpu/o3/issue_queue.cc |
Adds vector memory ready-queue insertion, issue handling, and cleanup on commit/squash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool bypassReady = true; | ||
| for (int i = 0; i < inst->numSrcRegs(); i++) { | ||
| auto src = inst->renamedSrcIdx(i); | ||
| if (src->isFixedMapping()) { | ||
| continue; | ||
| } | ||
| if (!scheduler->bypassScoreboard[src->flatIndex()]) { | ||
| bypassReady = false; | ||
| break; | ||
| } | ||
| } | ||
| if (!bypassReady) { | ||
| // Keep FIFO order in vectorReadyQ and retry in later cycles. | ||
| break; |
| addToFu(inst); | ||
| cpu->perfCCT->updateInstPos(inst->seqNum, PerfRecord::AtIssueReadReg); |
![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 (2)
src/cpu/o3/issue_queue.cc (2)
618-618: 💤 Low valueMinor formatting: missing space before brace.
The
else{should have a space before the opening brace to follow standard C++ formatting conventions.🔧 Suggested fix
- else{ + else {🤖 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/o3/issue_queue.cc` at line 618, Replace the malformed "else{" with the properly spaced "else {" in the affected branch (update the `else` statement that currently reads "else{" to "else {") to conform to the project's C++ formatting conventions; locate the `else{` token in src/cpu/o3/issue_queue.cc and adjust the spacing only.
528-539: 💤 Low valueConsider including
vectorReadyQin theidle()check.The
idle()method checksreadyQsandreplayQbut not the newvectorReadyQ. If there are instructions pending invectorReadyQwhile the other queues are empty, the IQ may incorrectly report as not-idle.🔧 Suggested fix
bool IssueQue::idle() { bool idle = false; for (auto it : readyQs) { if (it->size()) { idle = true; } } idle |= replayQ.size() > 0; + idle |= vectorReadyQ.size() > 0; return idle; }🤖 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/o3/issue_queue.cc` around lines 528 - 539, IssueQue::idle currently only checks readyQs and replayQ and misses vectorReadyQ, so IQ can report idle when vectorReadyQ has entries; update IssueQue::idle to include a check for vectorReadyQ (e.g., test vectorReadyQ.size() > 0 or !vectorReadyQ.empty()) and OR that result into the existing idle/replayQ logic so any pending entries in vectorReadyQ mark the queue as non-idle; refer to the IssueQue::idle method and the readyQs, replayQ and vectorReadyQ members when making the change.
🤖 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/o3/issue_queue.cc`:
- Around line 835-856: popReadyVectorInst can return canceled instructions
because it only checks for null and isSquashed; add a canceled check like
issueToFu does. In the IssueQue::popReadyVectorInst function, after confirming
inst is non-null and before erasing vectorReadyQSeqs/returning, call
inst->canceled() and continue the loop if true so canceled instructions are
skipped (mirroring the filtering in issueToFu and keeping vectorReadyQSeqs
handling consistent).
---
Nitpick comments:
In `@src/cpu/o3/issue_queue.cc`:
- Line 618: Replace the malformed "else{" with the properly spaced "else {" in
the affected branch (update the `else` statement that currently reads "else{" to
"else {") to conform to the project's C++ formatting conventions; locate the
`else{` token in src/cpu/o3/issue_queue.cc and adjust the spacing only.
- Around line 528-539: IssueQue::idle currently only checks readyQs and replayQ
and misses vectorReadyQ, so IQ can report idle when vectorReadyQ has entries;
update IssueQue::idle to include a check for vectorReadyQ (e.g., test
vectorReadyQ.size() > 0 or !vectorReadyQ.empty()) and OR that result into the
existing idle/replayQ logic so any pending entries in vectorReadyQ mark the
queue as non-idle; refer to the IssueQue::idle method and the readyQs, replayQ
and vectorReadyQ members when making the change.
🪄 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: e5465c34-d870-491f-9bec-f596041d29cf
📒 Files selected for processing (2)
src/cpu/o3/issue_queue.ccsrc/cpu/o3/issue_queue.hh
Temporarily delay the splitting of all vector memory access instructions by three cycles, and let them compete with replayed instructions and normally issued instructions for the load/store pipeline. This part of the logic will be further refined based on the instruction type, followed by detailed calibration with the RTL. Change-Id: I51ee33bae840cd3eebf2e761bfc3ebdf2f6309a4
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cpu/o3/issue_queue.cc (1)
474-479: 💤 Low valueInstruction lost if
checkScoreboardfails after pop.The instruction is already erased from
vectorReadyQSeqs(line 474) and popped fromvectorDelayedReadyQ(line 475) beforecheckScoreboardis called. IfcheckScoreboardreturns false, the instruction is simply skipped viacontinueand never re-queued—effectively lost.However, since the
bypassReadyloop (lines 448-462) already verifies all source registers againstbypassScoreboard, andcheckScoreboardalso checksbypassScoreboard, the latter should never fail if the former passed. Consider either:
- Removing the redundant
checkScoreboardcall, or- Moving the
checkScoreboardcall before the pop and breaking (similar tobypassReady) so the instruction remains in the queue for retry.🤖 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/o3/issue_queue.cc` around lines 474 - 479, The current code erases inst->seqNum from vectorReadyQSeqs and pops vectorDelayedReadyQ before calling checkScoreboard, which can drop an instruction if checkScoreboard returns false; fix by either removing the redundant checkScoreboard call in this bypassReady handling (since bypassReady already validated bypassScoreboard) or move the checkScoreboard invocation to before modifying vectorReadyQSeqs/vectorDelayedReadyQ and, if it fails, break/leave the instruction in the queue (mirror the bypassReady behavior) so the instruction is retried; update the logic around vectorReadyQSeqs, vectorDelayedReadyQ, and checkScoreboard accordingly.
🤖 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.
Nitpick comments:
In `@src/cpu/o3/issue_queue.cc`:
- Around line 474-479: The current code erases inst->seqNum from
vectorReadyQSeqs and pops vectorDelayedReadyQ before calling checkScoreboard,
which can drop an instruction if checkScoreboard returns false; fix by either
removing the redundant checkScoreboard call in this bypassReady handling (since
bypassReady already validated bypassScoreboard) or move the checkScoreboard
invocation to before modifying vectorReadyQSeqs/vectorDelayedReadyQ and, if it
fails, break/leave the instruction in the queue (mirror the bypassReady
behavior) so the instruction is retried; update the logic around
vectorReadyQSeqs, vectorDelayedReadyQ, and checkScoreboard accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d418bc27-4644-4d76-8ce0-bfb4d47296dd
📒 Files selected for processing (2)
src/cpu/o3/issue_queue.ccsrc/cpu/o3/issue_queue.hh
Summary by CodeRabbit