matrix: Add O3 path and detailed CUTE backend for XSAI GEMM#864
Open
sada22222 wants to merge 18 commits into
Open
matrix: Add O3 path and detailed CUTE backend for XSAI GEMM#864sada22222 wants to merge 18 commits into
sada22222 wants to merge 18 commits into
Conversation
added 6 commits
May 22, 2026 18:10
Change-Id: Ic9d65ab3950475b6b3b9f728a60c86ee46c6e9c7
Change-Id: Id2655b139d572c117ebd3dd2749b0375f9fd64eb
This reverts commit 668143e.
Change-Id: I261ecb7c2acfbfe58bdc3c7c0032d166e5f76bd4
Change-Id: Ia4dda7ce0ee0c5c20098f891a851c90358a7cb55
Change-Id: Ie89407d610ad00238a7b251e064ab0e97397e964
|
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 RISC‑V matrix ISA/CSR decode and per-instruction matrix metadata; wires RMisc registers and matrix renamed tile regs; extends O3 (DynInst, rename, IEW, issue queues, LSQ/MLS, ROB, commit) to stage/pipeline matrix payloads and AMU buffering; implements DetailedCuteBackend, register-resource/scoreboard, LocalMMU/fill-table, timing-memory adapters, tests, and documentation. ChangesRISC-V Matrix O3 + CUTE backend integration
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/arch/riscv/isa/decoder.isa (1)
2287-2464:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the new matrix CSR aliases non-speculative and serializing.
These
mtile*CSR variants write architectural state throughMatrixCSRCarrierExecute, but unlike the neighboringcsrrw/csrrs/csrrccases they are not flaggedIsSerializeAfter/IsNonSpeculative. That lets tile-shape updates issue like regular ALU ops and leak squashed writes before commit.🤖 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/arch/riscv/isa/decoder.isa` around lines 2287 - 2464, The Matrix CSR alias op entries (MatrixCarrierRs1WriteCSROp::csrrw_mtilem/mtile n/mtilek, MatrixCarrierRs1SetCSROp::csrrs_*, MatrixCarrierRs1ClearCSROp::csrrc_*, MatrixCarrierUimmWriteCSROp::csrrwi_*, MatrixCarrierUimmSetCSROp::csrrsi_*, MatrixCarrierUimmClearCSROp::csrrci_*) must be marked serializing and non‑speculative; update each of those decode lines to include the IsSerializeAfter and IsNonSpeculative flags (the same way csrrw_s / csrrs_s / csrrc_s etc. are annotated) so these MatrixCSRCarrierExecute writes cannot be speculatively executed or reordered.src/cpu/o3/issue_queue.cc (1)
614-655:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse per-class RMisc operand ordinals here.
The new RMisc port lookup uses the global operand index
i. For mixed-class instructions, the first RMisc src/dst can appear ati > 0, which means no RMisc port gets reserved at all and arbitration is silently skipped.Proposed fix
- for (int i = 0; i < inst->numDestRegs(); i++) { + int r_misc_dest_idx = 0; + for (int i = 0; i < inst->numDestRegs(); i++) { auto pdst = inst->renamedDestIdx(i); if (pdst->isFixedMapping()) [[unlikely]] continue; std::pair<int, int> rfTypePortId; // write port is point to point with dstid if (pdst->isIntReg() && intWrRfTPI[pi].size() > i) { rfTypePortId = intWrRfTPI[pi][i]; scheduler->useRfWrPort(inst, pdst, rfTypePortId.first, rfTypePortId.second); - } else if (pdst->is(RMiscRegClass) && - rMiscWrRfTPI[pi].size() > i) { - rfTypePortId = rMiscWrRfTPI[pi][i]; - scheduler->useRfWrPort( - inst, pdst, rfTypePortId.first, rfTypePortId.second); + } else if (pdst->is(RMiscRegClass)) { + if (rMiscWrRfTPI[pi].size() > r_misc_dest_idx) { + rfTypePortId = rMiscWrRfTPI[pi][r_misc_dest_idx]; + scheduler->useRfWrPort( + inst, pdst, rfTypePortId.first, rfTypePortId.second); + } + ++r_misc_dest_idx; } } - - // get regfile read port - for (int i = 0; i < inst->numSrcRegs(); i++) { + // get regfile read port + int r_misc_src_idx = 0; + for (int i = 0; i < inst->numSrcRegs(); i++) { PhysRegIdPtr psrc = inst->renamedSrcIdx(i); if (psrc->isFixedMapping()) continue; std::pair<int, int> rfTypePortId; // read port is point to point with srcid @@ - } else if (psrc->is(RMiscRegClass) && - rMiscRdRfTPI[pi].size() > i) { - rfTypePortId = rMiscRdRfTPI[pi][i]; - scheduler->useRfRdPort( - inst, psrc, rfTypePortId.first, rfTypePortId.second); + } else if (psrc->is(RMiscRegClass)) { + if (rMiscRdRfTPI[pi].size() > r_misc_src_idx) { + rfTypePortId = rMiscRdRfTPI[pi][r_misc_src_idx]; + scheduler->useRfRdPort( + inst, psrc, rfTypePortId.first, rfTypePortId.second); + } + ++r_misc_src_idx; } }🤖 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 614 - 655, The code incorrectly indexes RMisc port tables (rMiscWrRfTPI, rMiscRdRfTPI) with the global operand index i, so mixed-class operands skip RMisc ports; fix by tracking a per-class RMisc ordinal (e.g., rmisc_dst_idx for dest loop and rmisc_src_idx for src loop) that increments only when inst->renamedDestIdx(i)/renamedSrcIdx(i) is RMiscRegClass, and use that ordinal to index rMiscWrRfTPI[rp][rmisc_dst_idx] and rMiscRdRfTPI[rp][rmisc_src_idx] when calling scheduler->useRfWrPort/useRfRdPort; also ensure the TX dynamic port optimization (enableMainRdpOpt branch) still references the correct RMisc ordinal for borrowing if src0 is RMisc or other classes.
🧹 Nitpick comments (6)
src/cpu/o3/lsq.hh (1)
839-848: ⚡ Quick winRename new LSQ matrix APIs to lower_snake_case.
The new method names use mixedCase, which is inconsistent with the naming rule for methods. Please rename these APIs (and corresponding call sites) to lower_snake_case for guideline compliance.
As per coding guidelines, "Functions and methods should use lower_snake_case naming convention".
🤖 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/lsq.hh` around lines 839 - 848, Rename the mixedCase LSQ matrix API methods to lower_snake_case: change issueMatrixMem -> issue_matrix_mem, canAllocateMatrixMem -> can_allocate_matrix_mem, hasMatrixMemEntry -> has_matrix_mem_entry, allocateMatrixMemEntry -> allocate_matrix_mem_entry, scheduleMatrixMemReplay -> schedule_matrix_mem_replay, squashMatrixMem -> squash_matrix_mem, retireCommittedMatrixMem -> retire_committed_matrix_mem, matrixReplayQueue -> matrix_replay_queue, and matrixVirtualQueue -> matrix_virtual_queue; update the corresponding declarations, definitions, all call sites, any overrides/virtuals, and tests to use the new names so symbols match across header/implementation and usages.src/cpu/o3/lsq_unit.hh (1)
343-344: ⚡ Quick winUse lower_snake_case for new LSQUnit methods.
issueMatrixMemandmatrixReplayReadyshould be renamed to lower_snake_case to match project method naming rules.As per coding guidelines, "Functions and methods should use lower_snake_case naming convention".
🤖 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/lsq_unit.hh` around lines 343 - 344, Rename the two new LSQUnit methods to follow lower_snake_case naming: change MlsUnit::issueMatrixMem to MlsUnit::issue_matrix_mem and MlsUnit::matrixReplayReady to MlsUnit::matrix_replay_ready, and update all declarations/usages and any references to MlsReplayQueue::ReplayState accordingly (e.g., function signatures, callers, tests, and forward declarations in lsq_unit.hh and implementation files) to use the new names to keep the code consistent with the project's naming convention.src/matrix/LocalMMUModel.hh (1)
81-103: 🏗️ Heavy liftMethod names should be lower_snake_case per repo rule.
Examples: Line 83 (
takeReadyResponses), Line 85 (pendingCount), Line 102 (allocateSource), Line 103 (freeSource).As per coding guidelines, "Functions and methods should use lower_snake_case naming convention".
🤖 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/matrix/LocalMMUModel.hh` around lines 81 - 103, Rename the public and private methods to lower_snake_case to follow repo convention: change takeReadyResponses -> take_ready_responses, pendingCount -> pending_count, outstandingCount -> outstanding_count, readyCount -> ready_count, issuedCount -> issued_count, clientIndex -> client_index, takeNextRequest -> take_next_request, allocateSource -> allocate_source, and freeSource -> free_source; update their declarations in LocalMMUModel.hh and rename all corresponding definitions and call sites (including uses in tests/other modules) so signatures and visibility remain identical except for the new names.src/matrix/MRegFile.hh (1)
64-79: 🏗️ Heavy liftMatrixRegFile method names do not follow lower_snake_case.
Examples: Line 64 (
abRegCount), Line 68 (hasRegister), Line 75 (hasAllocatedState).As per coding guidelines, "Functions and methods should use lower_snake_case naming convention".
🤖 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/matrix/MRegFile.hh` around lines 64 - 79, Rename all MatrixRegFile public methods to lower_snake_case: change abRegCount -> ab_reg_count, cRegCount -> c_reg_count, regCount -> reg_count, hasRegister -> has_register, read -> read (if you want to keep semantics but ensure snake_case if different), write -> write (ensure naming), zero -> zero (ensure snake_case), allocated -> allocated (ensure snake_case), hasAllocatedState -> has_allocated_state, and the private bank(...) overloads if they violate casing. Update the corresponding method declarations and all definitions/usages (call sites, tests, and any overrides) to the new names (e.g., MatrixRegFile::ab_reg_count, etc.) and run the build to fix any remaining unresolved symbol errors.src/matrix/CUTETOP.hh (1)
96-107: 🏗️ Heavy liftPublic API and internal methods violate required lower_snake_case naming.
Examples include Line 210 (
canAccept), Line 217 (stepis fine), Line 341 (releaseReady), Line 355 (processFifoHead), and Line 423 (loadRuntimeTile).As per coding guidelines, "Functions and methods should use lower_snake_case naming convention".
Also applies to: 210-423
🤖 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/matrix/CUTETOP.hh` around lines 96 - 107, Public API and internal methods use camelCase/MixedCase instead of the required lower_snake_case; rename each symbol and update all references. For example rename makeRead/makeWrite -> make_read/make_write, advanceCycle -> advance_cycle, readResponseReady -> read_response_ready, consumeReadResponse -> consume_read_response, currentCycle -> current_cycle, canAccept -> can_accept, releaseReady -> release_ready, processFifoHead -> process_fifo_head, loadRuntimeTile -> load_runtime_tile (and any other camelCase symbols between lines ~210-423). Update declarations in headers (e.g., CUTETOP.hh), corresponding definitions, all callers, tests, and any public documentation or ABI-stable exports to use the new lower_snake_case names while keeping behavior unchanged.src/matrix/CUTEParameters.hh (1)
156-205: 🏗️ Heavy liftRename public helpers/factories to lower_snake_case for consistency.
Line 156 (
makeArithZero), Line 174 (makeRelease), Line 185 (makeMma), and Line 287 (elemBytes) do not follow the required function naming convention.As per coding guidelines, "Functions and methods should use lower_snake_case naming convention".
Also applies to: 286-287
🤖 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/matrix/CUTEParameters.hh` around lines 156 - 205, The public factory functions and helper (makeArithZero, makeRelease, makeMma) and the elemBytes symbol must be renamed to lower_snake_case to follow the project's naming convention; rename makeArithZero -> make_arith_zero, makeRelease -> make_release, makeMma -> make_mma, and elemBytes -> elem_bytes in their declarations and definitions (e.g., the static functions in CuteParameters.hh) and update every callsite, forward declaration, header include, and any tests or documentation to the new names so compilation and linkage remain correct.
🤖 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 `@configs/common/FUScheduler.py`:
- Around line 27-32: The helper functions RMiscRD and the other helper at lines
~41 use the builtin name id as a parameter which shadows Python's id and
triggers lint A002; rename that parameter to port_id (or similar) in both
function signatures and update all internal references (e.g., replace id << 2
with port_id << 2) so the logic is unchanged but the builtin is not shadowed;
ensure any local callers in the same file that pass or reference the old name
are updated to the new parameter name as well.
In `@src/arch/riscv/insts/matrix_static_info.hh`:
- Around line 359-375: The decoded width from mach_inst.rd (RD[4:3]) used in the
case 0x02 and 0x0a paths should be validated before calling setLsu: do not let
matrixAbElemKind/matrixAccessSize silently treat unknown encodings as Int8—check
the decoded width against the supported AB load widths (only the allowed
encodings) and, if it's not one of them, mark the instruction info as invalid
(or return/fail decoding) instead of calling setLsu; update the logic around the
case handlers (where width is computed and setLsu is invoked) to perform this
validation and handle invalid widths early.
In `@src/arch/riscv/isa/decoder.isa`:
- Around line 2791-2817: The fallback path in MatrixCarrierOp::mzero builds a
MatrixArithStubRequest with only op/md, losing mtilem/mtilen from the earlier
ExecContext::MatrixExecPayload; when stageMatrixExecPayload(payload) returns
false, copy the payload tile dimensions (payload.mtilem and payload.mtilen) into
the request (e.g., req.mtilem = payload.mtilem; req.mtilen = payload.mtilen;)
before calling RiscvISA::ISA::recordMatrixArithRequest, and also propagate any
other relevant payload fields used at execution time (e.g., elemType) so the
deferred zero op uses the original tile shape.
- Around line 2584-2611: The fallback paths in MatrixConfOp::msyncregreset and
MatrixConfOp::mrelease call isa->resetMatrixToken(RS2) /
isa->releaseMatrixToken(RS2) when xc->stage... fails, but those ops are not
marked non‑speculative; make these ops non‑speculative so they cannot be
executed on a speculative fallback (or alternatively avoid touching ISA token
state in the fallback and instead queue a non‑speculative commit). Concretely,
update the decoder entries that construct MatrixConfOp::msyncregreset and
MatrixConfOp::mrelease to include the IsNonSpeculative flag (or move the ISA
calls into a non‑speculative cleanup path), referencing the decoder ops
MatrixSyncOp / MatrixReleaseOp and the methods isa->resetMatrixToken and
isa->releaseMatrixToken.
In `@src/arch/riscv/regs/misc.hh`:
- Around line 490-492: The CSR encodings chosen for CSR_MTILEM, CSR_MTILEN, and
CSR_MTILEK are in the read-only space (csr[11:10] == 0b11) so any
csrrw/csrrs/csrrc will fault; update the three constants (CSR_MTILEM,
CSR_MTILEN, CSR_MTILEK) to use architecturally writable encodings whose bits
11:10 are not 0b11 (i.e., pick encodings with csr[11:10] == 00, 01, or 10) so
the new CSR read/write paths can modify the tile registers successfully. Ensure
the new values do not collide with other CSR definitions in the file.
In `@src/arch/riscv/regs/renameable_misc.hh`:
- Around line 20-23: The RMiscRegClass enum was unintentionally shortened by
removing the explicit NumRegs = 10, which causes NumRegs to evaluate to 6 and
renumber later entries; restore the original capacity by either adding back
"NumRegs = 10" or explicitly assigning numeric values to the new indices
(_MtilemIdx, _MtilenIdx, _MtilekIdx) so the enum retains the previous span and
the tail indices remain stable; update RMiscRegClass (and any references relying
on its size) to use the restored explicit size.
In `@src/cpu/o3/commit.cc`:
- Around line 2008-2016: amuEntryDemand currently increments for every
non-squashed entry in fixedbuffer[i], causing matrixAmuEntryBlock to
backpressure unrelated instructions; change the loop that computes
amuEntryDemand to only count instructions that actually allocate matrix AMU
state (e.g., replace the unconditional ++amuEntryDemand in the loop over
fixedbuffer[i] with a conditional like if (inst->usesMatrixAmu() ||
inst->willAllocateMatrixAmu() || check the instruction opcode/flags that
indicate matrix AMU allocation) ++amuEntryDemand), so matrixAmuEntryBlock calls
rob->numFreeMatrixAmuEntries(i) only consider true matrix-AMU users and avoid
blocking scalar/vector instructions.
In `@src/cpu/o3/dyn_inst.hh`:
- Around line 299-301: The reported uninitialized-payload concern can be
removed: keep ExecContext::MatrixExecPayload matrixExecPayload as-is because
DynInst's constructor (in src/cpu/o3/dyn_inst.cc) explicitly assigns
matrixExecPayload = {} and clearMatrixPayload() also sets matrixExecPayload =
{}, so matrixPayloadValid() will not observe indeterminate state; update the
review/remove the warning and any suggested in-class initializer change,
referencing ExecContext::MatrixExecPayload, matrixExecPayload, the DynInst
constructor, matrixPayloadValid(), and clearMatrixPayload() to justify the
removal.
In `@src/cpu/o3/lsq.cc`:
- Around line 338-345: The LSQ lifecycle currently ignores mlsReplayQueue and
mlsVirtualQueue causing premature drained reports and stale matrix entries
during takeover/switch; update isDrained() to include checks that both
mlsReplayQueue and mlsVirtualQueue are empty (in addition to classic LQ/SQ),
update drainSanityCheck() to assert/verify those queues contain no live entries
before declaring drained, and extend takeOverFrom() to transfer any outstanding
entries/state from the source's mlsReplayQueue and mlsVirtualQueue to the
destination and then clear the source queues (or reinitialize them) to avoid
carrying stale matrix entries across takeovers; apply same updates consistently
where classic queues are already handled (see existing LQ/SQ handling code
paths).
In `@src/cpu/o3/mls_unit.cc`:
- Around line 931-955: The code only allocates into replayQueue when
state.needReplay is true and replayQueue is non-null, so if replayQueue is null
the instruction silently falls through without replaying; update the branch
after runStage3 to explicitly handle the case where state.needReplay is true but
replayQueue is null by failing fast or scheduling an alternative: add an else
for "if (state.needReplay && replayQueue)" that calls panic (or returns an
appropriate error) with a clear message (include inst->threadNumber and
inst->seqNum), and ensure result.needReplay is set when a replay is required;
reference state.needReplay, replayQueue, replayQueue->allocateOrUpdate, and
result.needReplay to locate and modify the logic.
In `@src/cpu/o3/rob.cc`:
- Around line 299-317: The current shouldTrackMatrixAmu(const DynInstPtr &inst)
always returns true for any non-null instruction causing non-matrix ops to
consume AMU shadow capacity; change it to only return true for matrix
instructions that require AMU control (e.g. return inst && inst->isMatrixInst()
&& inst->matrixNeedAmuCtrl()); update insertMatrixAmuEntry to rely on that
predicate (so allocate is only called for matrix entries) and ensure the same
predicate is used in any duplicate logic elsewhere (e.g., the similar code
referenced around the other occurrence) to avoid shadow buffer inflation.
In `@src/matrix/CUTEParameters.hh`:
- Line 71: MatrixTensor::valid() currently returns true for partially-specified
tensors because it uses OR; change the check to require a fully-specified tensor
by using AND and verify element count matches shape: ensure rows > 0 && cols > 0
&& !elements.empty() and also elements.size() == static_cast<size_t>(rows *
cols) (use the fields rows, cols, elements and the valid() method to implement
this).
In `@src/matrix/CUTETOP.cc`:
- Around line 666-671: The code always queries MatrixBankKind::C when checking
and reading registers (regFile.hasRegister and regFile.read), which ignores the
store's actual source bank and can load the wrong data; change the logic to
select the correct MatrixBankKind from the task entry (use the entry's read-bank
selector such as task.entry.readBank or task.entry.readBanks[0] together with
task.entry.readRegs[0]), then call regFile.hasRegister(selectedBank,
task.entry.readRegs[0]) and regFile.read(selectedBank, task.entry.readRegs[0])
and assign the result to task.bufferedTensor / set task.hasBufferedTensor as
before, leaving task.bufferedCompletion.status handling unchanged.
In `@src/matrix/MemoryLoader.cc`:
- Around line 153-165: The storeTile method
(SparseMatrixMemoryAdapter::storeTile) currently validates
tensor.rows/cols/elemType but does not check the length of tensor.elements
before indexing, which can cause out-of-bounds reads; update storeTile to
compute the expected element count (e.g., expected = tensor.rows * tensor.cols)
and verify tensor.elements.size() >= expected before the nested r/c loop and
before any other indexed access (also apply the same check to the other
occurrence around the 241-250 block), returning false or signalling an error if
the payload is too short; keep the existing elemAddr usage and per-element
assignment logic unchanged once the size guard passes.
In `@src/matrix/MemoryLoader.hh`:
- Around line 48-50: The adapter method names in MemoryLoader.hh (e.g.,
loadTile, storeTile, writeElement, readElement and the other methods at lines
referenced) must be converted to lower_snake_case to follow repo conventions:
rename loadTile -> load_tile, storeTile -> store_tile, writeElement ->
write_element, readElement -> read_element (and apply the same lower_snake_case
change to the other methods referenced at 56-58, 64-69, 78-80). Update the
virtual declarations in the MemoryLoader class to the new names, then update all
corresponding implementations, overrides, and call sites to use the new names
and keep signatures identical (preserve parameter types/constness and add
override where appropriate) so the API remains functionally the same. Ensure you
run a full project-wide search/replace to catch all references and rebuild to
verify no missing symbols.
In `@src/matrix/Scoreboard.cc`:
- Around line 157-163: DetailedCuteScoreboard::onComputeIssue currently resets
fu then sets destination fields but never marks the compute FU as busy; update
the issue path to set fu.busy = true after resetFu(fu) (and if you only want to
mark occupancy when a destination is actually written, set fu.busy = true
conditionally when entry.writeValid[0] is true) so the compute FU occupancy
tracking (fu.busy) remains consistent with other FUs.
In `@src/matrix/Scoreboard.hh`:
- Around line 95-149: The API uses camelCase and mixed-case constants; rename
all methods to lower_snake_case and compile-time constants to ALL_CAPS to match
project conventions: e.g. canIssue->can_issue, blockReason->block_reason,
onLoadIssue->on_load_issue, onLoadFinish->on_load_finish,
onStoreIssue->on_store_issue, onStoreReadFinish->on_store_read_finish,
onStoreWriteFinish->on_store_write_finish, onComputeIssue->on_compute_issue,
onComputeReadFinishA/B/C->on_compute_read_finish_a/_b/_c,
onComputeWriteFinishC->on_compute_write_finish_c, onArithIssue->on_arith_issue,
onArithFinish->on_arith_finish, onIssue->on_issue,
fuBusyForTest->fu_busy_for_test, regBusyForTest->reg_busy_for_test,
pendingReadersForTest->pending_readers_for_test, fuState->fu_state,
isFuBusy->is_fu_busy, regStatus->reg_status, bankForSource->bank_for_source,
destBank->dest_bank, loadFu->load_fu, sourceReady->source_ready,
destBusy->dest_busy, sourceHasPendingReaders->source_has_pending_readers,
destHasPendingReaders->dest_has_pending_readers, resetSrc->reset_src,
resetFu->reset_fu, setupSrc->setup_src, reserveDest->reserve_dest,
releaseDest->release_dest, incrementPendingReader->increment_pending_reader,
decrementPendingReader->decrement_pending_reader,
wakeupConsumers->wakeup_consumers; and change SrcAIdx/SrcBIdx/SrcCIdx to
ALL_CAPS (e.g. SRC_A_IDX, SRC_B_IDX, SRC_C_IDX). Update all declarations and
corresponding definitions/usages to the new names.
In `@src/matrix/TaskController.cc`:
- Around line 76-79: Replace the debug-only assert in
DetailedCuteBackend::submit with a runtime capacity check: call canAccept(req)
and if it returns false, perform the appropriate runtime rejection/backpressure
action (e.g. increment a rejection counter such as counters.fifoRejected and
return or throw the error code used by this backend) instead of relying on
assert; update both occurrences around counters.fifoEnqueue (and the similar
block at lines 86-87) so capacity is enforced in release builds by explicit if
(!canAccept(req)) { handle rejection } before incrementing fifoEnqueue.
In `@src/matrix/TaskController.hh`:
- Around line 50-56: The header exposes methods using camelCase which violates
the repo naming rule; rename the virtual interface methods canAccept, submit,
hasWork, step, hasCompletion, popCompletion, and hasArchitecturalState to
lower_snake_case (e.g., can_accept, submit, has_work, step, has_completion,
pop_completion, has_architectural_state), then update all corresponding
implementations, overrides, callers, and tests to the new symbols; also apply
the same renaming to the other camelCase methods referenced around the file (the
methods at the other reported locations) so declarations and definitions stay in
sync and compilation/overrides remain correct.
---
Outside diff comments:
In `@src/arch/riscv/isa/decoder.isa`:
- Around line 2287-2464: The Matrix CSR alias op entries
(MatrixCarrierRs1WriteCSROp::csrrw_mtilem/mtile n/mtilek,
MatrixCarrierRs1SetCSROp::csrrs_*, MatrixCarrierRs1ClearCSROp::csrrc_*,
MatrixCarrierUimmWriteCSROp::csrrwi_*, MatrixCarrierUimmSetCSROp::csrrsi_*,
MatrixCarrierUimmClearCSROp::csrrci_*) must be marked serializing and
non‑speculative; update each of those decode lines to include the
IsSerializeAfter and IsNonSpeculative flags (the same way csrrw_s / csrrs_s /
csrrc_s etc. are annotated) so these MatrixCSRCarrierExecute writes cannot be
speculatively executed or reordered.
In `@src/cpu/o3/issue_queue.cc`:
- Around line 614-655: The code incorrectly indexes RMisc port tables
(rMiscWrRfTPI, rMiscRdRfTPI) with the global operand index i, so mixed-class
operands skip RMisc ports; fix by tracking a per-class RMisc ordinal (e.g.,
rmisc_dst_idx for dest loop and rmisc_src_idx for src loop) that increments only
when inst->renamedDestIdx(i)/renamedSrcIdx(i) is RMiscRegClass, and use that
ordinal to index rMiscWrRfTPI[rp][rmisc_dst_idx] and
rMiscRdRfTPI[rp][rmisc_src_idx] when calling scheduler->useRfWrPort/useRfRdPort;
also ensure the TX dynamic port optimization (enableMainRdpOpt branch) still
references the correct RMisc ordinal for borrowing if src0 is RMisc or other
classes.
---
Nitpick comments:
In `@src/cpu/o3/lsq_unit.hh`:
- Around line 343-344: Rename the two new LSQUnit methods to follow
lower_snake_case naming: change MlsUnit::issueMatrixMem to
MlsUnit::issue_matrix_mem and MlsUnit::matrixReplayReady to
MlsUnit::matrix_replay_ready, and update all declarations/usages and any
references to MlsReplayQueue::ReplayState accordingly (e.g., function
signatures, callers, tests, and forward declarations in lsq_unit.hh and
implementation files) to use the new names to keep the code consistent with the
project's naming convention.
In `@src/cpu/o3/lsq.hh`:
- Around line 839-848: Rename the mixedCase LSQ matrix API methods to
lower_snake_case: change issueMatrixMem -> issue_matrix_mem,
canAllocateMatrixMem -> can_allocate_matrix_mem, hasMatrixMemEntry ->
has_matrix_mem_entry, allocateMatrixMemEntry -> allocate_matrix_mem_entry,
scheduleMatrixMemReplay -> schedule_matrix_mem_replay, squashMatrixMem ->
squash_matrix_mem, retireCommittedMatrixMem -> retire_committed_matrix_mem,
matrixReplayQueue -> matrix_replay_queue, and matrixVirtualQueue ->
matrix_virtual_queue; update the corresponding declarations, definitions, all
call sites, any overrides/virtuals, and tests to use the new names so symbols
match across header/implementation and usages.
In `@src/matrix/CUTEParameters.hh`:
- Around line 156-205: The public factory functions and helper (makeArithZero,
makeRelease, makeMma) and the elemBytes symbol must be renamed to
lower_snake_case to follow the project's naming convention; rename makeArithZero
-> make_arith_zero, makeRelease -> make_release, makeMma -> make_mma, and
elemBytes -> elem_bytes in their declarations and definitions (e.g., the static
functions in CuteParameters.hh) and update every callsite, forward declaration,
header include, and any tests or documentation to the new names so compilation
and linkage remain correct.
In `@src/matrix/CUTETOP.hh`:
- Around line 96-107: Public API and internal methods use camelCase/MixedCase
instead of the required lower_snake_case; rename each symbol and update all
references. For example rename makeRead/makeWrite -> make_read/make_write,
advanceCycle -> advance_cycle, readResponseReady -> read_response_ready,
consumeReadResponse -> consume_read_response, currentCycle -> current_cycle,
canAccept -> can_accept, releaseReady -> release_ready, processFifoHead ->
process_fifo_head, loadRuntimeTile -> load_runtime_tile (and any other camelCase
symbols between lines ~210-423). Update declarations in headers (e.g.,
CUTETOP.hh), corresponding definitions, all callers, tests, and any public
documentation or ABI-stable exports to use the new lower_snake_case names while
keeping behavior unchanged.
In `@src/matrix/LocalMMUModel.hh`:
- Around line 81-103: Rename the public and private methods to lower_snake_case
to follow repo convention: change takeReadyResponses -> take_ready_responses,
pendingCount -> pending_count, outstandingCount -> outstanding_count, readyCount
-> ready_count, issuedCount -> issued_count, clientIndex -> client_index,
takeNextRequest -> take_next_request, allocateSource -> allocate_source, and
freeSource -> free_source; update their declarations in LocalMMUModel.hh and
rename all corresponding definitions and call sites (including uses in
tests/other modules) so signatures and visibility remain identical except for
the new names.
In `@src/matrix/MRegFile.hh`:
- Around line 64-79: Rename all MatrixRegFile public methods to
lower_snake_case: change abRegCount -> ab_reg_count, cRegCount -> c_reg_count,
regCount -> reg_count, hasRegister -> has_register, read -> read (if you want to
keep semantics but ensure snake_case if different), write -> write (ensure
naming), zero -> zero (ensure snake_case), allocated -> allocated (ensure
snake_case), hasAllocatedState -> has_allocated_state, and the private bank(...)
overloads if they violate casing. Update the corresponding method declarations
and all definitions/usages (call sites, tests, and any overrides) to the new
names (e.g., MatrixRegFile::ab_reg_count, etc.) and run the build to fix any
remaining unresolved symbol errors.
🪄 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: 83eb0d0c-1db1-4e87-ace5-1161d6b047b4
📒 Files selected for processing (57)
Matrix_Readme.mdconfigs/common/FUScheduler.pysrc/SConscriptsrc/arch/arm/isa.hhsrc/arch/riscv/insts/matrix_static_info.hhsrc/arch/riscv/insts/static_inst.hhsrc/arch/riscv/isa.ccsrc/arch/riscv/isa.hhsrc/arch/riscv/isa/decoder.isasrc/arch/riscv/isa/formats/formats.isasrc/arch/riscv/isa/formats/matrix_conf.isasrc/arch/riscv/isa/includes.isasrc/arch/riscv/regs/matrix.hhsrc/arch/riscv/regs/misc.hhsrc/arch/riscv/regs/renameable_misc.hhsrc/cpu/FuncUnit.pysrc/cpu/exec_context.hhsrc/cpu/o3/BaseO3CPU.pysrc/cpu/o3/FuncUnitConfig.pysrc/cpu/o3/SConscriptsrc/cpu/o3/commit.ccsrc/cpu/o3/cpu.ccsrc/cpu/o3/cpu.hhsrc/cpu/o3/dyn_inst.ccsrc/cpu/o3/dyn_inst.hhsrc/cpu/o3/iew.ccsrc/cpu/o3/iew.hhsrc/cpu/o3/issue_queue.ccsrc/cpu/o3/issue_queue.hhsrc/cpu/o3/lsq.ccsrc/cpu/o3/lsq.hhsrc/cpu/o3/lsq_unit.ccsrc/cpu/o3/lsq_unit.hhsrc/cpu/o3/matrix_amu_buffer.ccsrc/cpu/o3/matrix_amu_buffer.hhsrc/cpu/o3/mls_unit.ccsrc/cpu/o3/mls_unit.hhsrc/cpu/o3/rename.ccsrc/cpu/o3/rob.ccsrc/cpu/o3/rob.hhsrc/cpu/op_class.hhsrc/matrix/CUTEParameters.hhsrc/matrix/CUTETOP.ccsrc/matrix/CUTETOP.hhsrc/matrix/LocalMMUModel.ccsrc/matrix/LocalMMUModel.hhsrc/matrix/MRegFile.ccsrc/matrix/MRegFile.hhsrc/matrix/MatrixTE.ccsrc/matrix/MatrixTE.hhsrc/matrix/MemoryLoader.ccsrc/matrix/MemoryLoader.hhsrc/matrix/SConscriptsrc/matrix/Scoreboard.ccsrc/matrix/Scoreboard.hhsrc/matrix/TaskController.ccsrc/matrix/TaskController.hh
💤 Files with no reviewable changes (1)
- src/arch/riscv/isa/formats/formats.isa
Comment on lines
+27
to
+32
| def RMiscRD(id, p): | ||
| # [7:6] [5:2] [1:0] | ||
| assert id < 16 | ||
| assert p < 4 | ||
| ret = (2 << 6) | (id << 2) | (p) | ||
| return ret |
There was a problem hiding this comment.
Avoid shadowing Python builtin id in new helper APIs.
Line 27 and Line 41 use id as a parameter name; Ruff flags this (A002) and it can block lint checks. Rename to port_id (or similar) in both helpers.
Suggested patch
-def RMiscRD(id, p):
+def RMiscRD(port_id, p):
# [7:6] [5:2] [1:0]
- assert id < 16
+ assert port_id < 16
assert p < 4
- ret = (2 << 6) | (id << 2) | (p)
+ ret = (2 << 6) | (port_id << 2) | (p)
return ret
@@
-def RMiscWR(id, p):
+def RMiscWR(port_id, p):
# [8] [7:6] [5:2] [1:0]
- assert id < 16
+ assert port_id < 16
assert p < 4
- ret = (1 << 8) | (2 << 6) | (id << 2) | (p)
+ ret = (1 << 8) | (2 << 6) | (port_id << 2) | (p)
return retAlso applies to: 41-46
🧰 Tools
🪛 Ruff (0.15.14)
[error] 27-27: Function argument id is shadowing a Python builtin
(A002)
🤖 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/common/FUScheduler.py` around lines 27 - 32, The helper functions
RMiscRD and the other helper at lines ~41 use the builtin name id as a parameter
which shadows Python's id and triggers lint A002; rename that parameter to
port_id (or similar) in both function signatures and update all internal
references (e.g., replace id << 2 with port_id << 2) so the logic is unchanged
but the builtin is not shadowed; ensure any local callers in the same file that
pass or reference the old name are updated to the new parameter name as well.
Comment on lines
+359
to
+375
| case 0x02: | ||
| { | ||
| const auto width = static_cast<uint8_t>((mach_inst.rd >> 3) & 0x3); | ||
| setLsu(info, true, false, true, false, false, false, width, | ||
| matrixAbElemKind(width), matrixAccessSize(width), | ||
| MatrixStateOperand::Mtilem, | ||
| MatrixStateOperand::Mtilek, false); | ||
| break; | ||
| } | ||
| case 0x0a: | ||
| { | ||
| const auto width = static_cast<uint8_t>((mach_inst.rd >> 3) & 0x3); | ||
| setLsu(info, true, false, false, true, false, true, width, | ||
| matrixAbElemKind(width), matrixAccessSize(width), | ||
| MatrixStateOperand::Mtilek, | ||
| MatrixStateOperand::Mtilen, false); | ||
| break; |
There was a problem hiding this comment.
Reject unsupported AB load widths instead of silently treating them as Int8.
Line 361 and Line 370 decode RD[4:3] width, but widths outside e8/e16 currently fall through as Int8 (matrixAbElemKind/matrixAccessSize). This can mark invalid encodings as valid matrix LSU instructions.
Suggested patch
case 0x02:
{
const auto width = static_cast<uint8_t>((mach_inst.rd >> 3) & 0x3);
+ if (width != MatrixSewE8 && width != MatrixSewE16) {
+ info.valid = false;
+ break;
+ }
setLsu(info, true, false, true, false, false, false, width,
matrixAbElemKind(width), matrixAccessSize(width),
MatrixStateOperand::Mtilem,
MatrixStateOperand::Mtilek, false);
break;
}
case 0x0a:
{
const auto width = static_cast<uint8_t>((mach_inst.rd >> 3) & 0x3);
+ if (width != MatrixSewE8 && width != MatrixSewE16) {
+ info.valid = false;
+ break;
+ }
setLsu(info, true, false, false, true, false, true, width,
matrixAbElemKind(width), matrixAccessSize(width),
MatrixStateOperand::Mtilek,
MatrixStateOperand::Mtilen, false);
break;
}📝 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.
Suggested change
| case 0x02: | |
| { | |
| const auto width = static_cast<uint8_t>((mach_inst.rd >> 3) & 0x3); | |
| setLsu(info, true, false, true, false, false, false, width, | |
| matrixAbElemKind(width), matrixAccessSize(width), | |
| MatrixStateOperand::Mtilem, | |
| MatrixStateOperand::Mtilek, false); | |
| break; | |
| } | |
| case 0x0a: | |
| { | |
| const auto width = static_cast<uint8_t>((mach_inst.rd >> 3) & 0x3); | |
| setLsu(info, true, false, false, true, false, true, width, | |
| matrixAbElemKind(width), matrixAccessSize(width), | |
| MatrixStateOperand::Mtilek, | |
| MatrixStateOperand::Mtilen, false); | |
| break; | |
| case 0x02: | |
| { | |
| const auto width = static_cast<uint8_t>((mach_inst.rd >> 3) & 0x3); | |
| if (width != MatrixSewE8 && width != MatrixSewE16) { | |
| info.valid = false; | |
| break; | |
| } | |
| setLsu(info, true, false, true, false, false, false, width, | |
| matrixAbElemKind(width), matrixAccessSize(width), | |
| MatrixStateOperand::Mtilem, | |
| MatrixStateOperand::Mtilek, false); | |
| break; | |
| } | |
| case 0x0a: | |
| { | |
| const auto width = static_cast<uint8_t>((mach_inst.rd >> 3) & 0x3); | |
| if (width != MatrixSewE8 && width != MatrixSewE16) { | |
| info.valid = false; | |
| break; | |
| } | |
| setLsu(info, true, false, false, true, false, true, width, | |
| matrixAbElemKind(width), matrixAccessSize(width), | |
| MatrixStateOperand::Mtilek, | |
| MatrixStateOperand::Mtilen, false); | |
| break; | |
| } |
🤖 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/arch/riscv/insts/matrix_static_info.hh` around lines 359 - 375, The
decoded width from mach_inst.rd (RD[4:3]) used in the case 0x02 and 0x0a paths
should be validated before calling setLsu: do not let
matrixAbElemKind/matrixAccessSize silently treat unknown encodings as Int8—check
the decoded width against the supported AB load widths (only the allowed
encodings) and, if it's not one of them, mark the instruction info as invalid
(or return/fail decoding) instead of calling setLsu; update the logic around the
case handlers (where width is computed and setLsu is invoked) to perform this
validation and handle invalid widths early.
Comment on lines
+2584
to
+2611
| 0x0: MatrixConfOp::msyncregreset({{ | ||
| if (!xc->stageMatrixTokenReset(RS2)) { | ||
| auto *isa = static_cast<RiscvISA::ISA *>( | ||
| xc->tcBase()->getIsaPtr()); | ||
| isa->resetMatrixToken(RS2); | ||
| } | ||
| }}, IsSerializeAfter, MatrixSyncOp); | ||
| } | ||
| } | ||
| } | ||
| 0x06: decode RS2 { | ||
| 0x0: decode RS1 { | ||
| 0x0: decode FUNCT3 { | ||
| 0x0: decode RD { | ||
| 0x4: MatrixConfOp::mzero1r({{ | ||
| auto *isa = dynamic_cast<RiscvISA::ISA *>(xc->tcBase()->getIsaPtr()); | ||
| assert(isa != nullptr); | ||
| isa->matrixZeroAcc(); | ||
| }}, No_OpClass); | ||
| } | ||
| 0x48: decode RS1 { | ||
| 0x0: decode FUNCT3 { | ||
| 0x0: decode RD { | ||
| 0x0: MatrixConfOp::mrelease({{ | ||
| // Phase 1 stub: collapse the post-commit backend | ||
| // release event into this conservative | ||
| // non-speculative execute point. | ||
| ExecContext::MatrixExecPayload payload; | ||
| payload.valid = true; | ||
| payload.kind = | ||
| ExecContext::MatrixExecPayload::Kind::Release; | ||
| payload.tokenIndex = RS2; | ||
| if (!xc->stageMatrixExecPayload(payload)) { | ||
| auto *isa = static_cast<RiscvISA::ISA *>( | ||
| xc->tcBase()->getIsaPtr()); | ||
| isa->releaseMatrixToken(RS2); | ||
| } | ||
| }}, IsSerializeAfter, MatrixReleaseOp); |
There was a problem hiding this comment.
Don't mutate matrix tokens from a speculative fallback path.
msyncregreset and mrelease call isa->resetMatrixToken() / isa->releaseMatrixToken() directly when staging is unavailable, but neither instruction is marked IsNonSpeculative. A squash after execute can therefore leave token state permanently updated.
Suggested flag fix
- }}, IsSerializeAfter, MatrixSyncOp);
+ }}, IsSerializeAfter, IsNonSpeculative, MatrixSyncOp);
...
- }}, IsSerializeAfter, MatrixReleaseOp);
+ }}, IsSerializeAfter, IsNonSpeculative, MatrixReleaseOp);📝 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.
Suggested change
| 0x0: MatrixConfOp::msyncregreset({{ | |
| if (!xc->stageMatrixTokenReset(RS2)) { | |
| auto *isa = static_cast<RiscvISA::ISA *>( | |
| xc->tcBase()->getIsaPtr()); | |
| isa->resetMatrixToken(RS2); | |
| } | |
| }}, IsSerializeAfter, MatrixSyncOp); | |
| } | |
| } | |
| } | |
| 0x06: decode RS2 { | |
| 0x0: decode RS1 { | |
| 0x0: decode FUNCT3 { | |
| 0x0: decode RD { | |
| 0x4: MatrixConfOp::mzero1r({{ | |
| auto *isa = dynamic_cast<RiscvISA::ISA *>(xc->tcBase()->getIsaPtr()); | |
| assert(isa != nullptr); | |
| isa->matrixZeroAcc(); | |
| }}, No_OpClass); | |
| } | |
| 0x48: decode RS1 { | |
| 0x0: decode FUNCT3 { | |
| 0x0: decode RD { | |
| 0x0: MatrixConfOp::mrelease({{ | |
| // Phase 1 stub: collapse the post-commit backend | |
| // release event into this conservative | |
| // non-speculative execute point. | |
| ExecContext::MatrixExecPayload payload; | |
| payload.valid = true; | |
| payload.kind = | |
| ExecContext::MatrixExecPayload::Kind::Release; | |
| payload.tokenIndex = RS2; | |
| if (!xc->stageMatrixExecPayload(payload)) { | |
| auto *isa = static_cast<RiscvISA::ISA *>( | |
| xc->tcBase()->getIsaPtr()); | |
| isa->releaseMatrixToken(RS2); | |
| } | |
| }}, IsSerializeAfter, MatrixReleaseOp); | |
| 0x0: MatrixConfOp::msyncregreset({{ | |
| if (!xc->stageMatrixTokenReset(RS2)) { | |
| auto *isa = static_cast<RiscvISA::ISA *>( | |
| xc->tcBase()->getIsaPtr()); | |
| isa->resetMatrixToken(RS2); | |
| } | |
| }}, IsSerializeAfter, IsNonSpeculative, MatrixSyncOp); | |
| } | |
| } | |
| } | |
| 0x48: decode RS1 { | |
| 0x0: decode FUNCT3 { | |
| 0x0: decode RD { | |
| 0x0: MatrixConfOp::mrelease({{ | |
| // Phase 1 stub: collapse the post-commit backend | |
| // release event into this conservative | |
| // non-speculative execute point. | |
| ExecContext::MatrixExecPayload payload; | |
| payload.valid = true; | |
| payload.kind = | |
| ExecContext::MatrixExecPayload::Kind::Release; | |
| payload.tokenIndex = RS2; | |
| if (!xc->stageMatrixExecPayload(payload)) { | |
| auto *isa = static_cast<RiscvISA::ISA *>( | |
| xc->tcBase()->getIsaPtr()); | |
| isa->releaseMatrixToken(RS2); | |
| } | |
| }}, IsSerializeAfter, IsNonSpeculative, MatrixReleaseOp); |
🤖 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/arch/riscv/isa/decoder.isa` around lines 2584 - 2611, The fallback paths
in MatrixConfOp::msyncregreset and MatrixConfOp::mrelease call
isa->resetMatrixToken(RS2) / isa->releaseMatrixToken(RS2) when xc->stage...
fails, but those ops are not marked non‑speculative; make these ops
non‑speculative so they cannot be executed on a speculative fallback (or
alternatively avoid touching ISA token state in the fallback and instead queue a
non‑speculative commit). Concretely, update the decoder entries that construct
MatrixConfOp::msyncregreset and MatrixConfOp::mrelease to include the
IsNonSpeculative flag (or move the ISA calls into a non‑speculative cleanup
path), referencing the decoder ops MatrixSyncOp / MatrixReleaseOp and the
methods isa->resetMatrixToken and isa->releaseMatrixToken.
Comment on lines
+2791
to
+2817
| 0x0: MatrixCarrierOp::mzero({{ | ||
| // src slot 0/1 carry Mtilem/Mtilen. | ||
| auto mtilem = xc->getRegOperand(this, 0); | ||
| auto mtilen = xc->getRegOperand(this, 1); | ||
| ExecContext::MatrixExecPayload payload; | ||
| payload.valid = true; | ||
| payload.kind = | ||
| ExecContext::MatrixExecPayload::Kind::Arith; | ||
| payload.op = 0x06; | ||
| payload.md = RD & 0x7; | ||
| payload.mtilem = mtilem; | ||
| payload.mtilen = mtilen; | ||
| payload.elemType = matrix::MatrixElemType::Int32; | ||
| if (!xc->stageMatrixExecPayload(payload)) { | ||
| auto *isa = static_cast<RiscvISA::ISA *>( | ||
| xc->tcBase()->getIsaPtr()); | ||
| RiscvISA::ISA::MatrixArithStubRequest req; | ||
| req.valid = true; | ||
| req.op = payload.op; | ||
| req.md = payload.md; | ||
| isa->recordMatrixArithRequest(req); | ||
| } | ||
| }}, {{ | ||
| setSrcRegIdx(_numSrcRegs++, MatrixRenamedTileMReg); | ||
| setSrcRegIdx(_numSrcRegs++, MatrixRenamedTileNReg); | ||
| }}, {{ | ||
| }}, MatrixArithOp); |
There was a problem hiding this comment.
Preserve tile dimensions in the mzero fallback request.
The staged path captures mtilem/mtilen, but the fallback MatrixArithStubRequest only records op and md. If stageMatrixExecPayload() returns false, the deferred zero op no longer carries its shape and can execute against a later tile configuration instead.
🤖 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/arch/riscv/isa/decoder.isa` around lines 2791 - 2817, The fallback path
in MatrixCarrierOp::mzero builds a MatrixArithStubRequest with only op/md,
losing mtilem/mtilen from the earlier ExecContext::MatrixExecPayload; when
stageMatrixExecPayload(payload) returns false, copy the payload tile dimensions
(payload.mtilem and payload.mtilen) into the request (e.g., req.mtilem =
payload.mtilem; req.mtilen = payload.mtilen;) before calling
RiscvISA::ISA::recordMatrixArithRequest, and also propagate any other relevant
payload fields used at execution time (e.g., elemType) so the deferred zero op
uses the original tile shape.
Comment on lines
+490
to
+492
| CSR_MTILEM = 0xCC5, | ||
| CSR_MTILEN = 0xCC6, | ||
| CSR_MTILEK = 0xCC7, |
There was a problem hiding this comment.
Pick writable CSR encodings for mtilem/mtilen/mtilek.
0xCC5-0xCC7 have csr[11:10] == 0b11, so they are architecturally read-only. The new csrrw/csrrs/csrrc paths added for these CSRs will therefore fault as soon as they try to change the value, leaving the tile CSR interface unwritable.
🤖 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/arch/riscv/regs/misc.hh` around lines 490 - 492, The CSR encodings chosen
for CSR_MTILEM, CSR_MTILEN, and CSR_MTILEK are in the read-only space
(csr[11:10] == 0b11) so any csrrw/csrrs/csrrc will fault; update the three
constants (CSR_MTILEM, CSR_MTILEN, CSR_MTILEK) to use architecturally writable
encodings whose bits 11:10 are not 0b11 (i.e., pick encodings with csr[11:10] ==
00, 01, or 10) so the new CSR read/write paths can modify the tile registers
successfully. Ensure the new values do not collide with other CSR definitions in
the file.
Comment on lines
+157
to
+163
| DetailedCuteScoreboard::onComputeIssue(const DecodedFifoEntry &entry) | ||
| { | ||
| auto &fu = fuState(FuKind::Compute); | ||
| resetFu(fu); | ||
| fu.destValid = entry.writeValid[0]; | ||
| fu.destBank = MatrixBankKind::C; | ||
| fu.destReg = entry.writeRegs[0]; |
There was a problem hiding this comment.
Mark compute FU busy on issue for consistent scoreboard state.
onComputeIssue is the only issue path that does not set fu.busy = true; this makes compute FU occupancy tracking inconsistent.
💡 Suggested fix
void
DetailedCuteScoreboard::onComputeIssue(const DecodedFifoEntry &entry)
{
auto &fu = fuState(FuKind::Compute);
resetFu(fu);
+ fu.busy = true;
fu.destValid = entry.writeValid[0];
fu.destBank = MatrixBankKind::C;
fu.destReg = entry.writeRegs[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.
Suggested change
| DetailedCuteScoreboard::onComputeIssue(const DecodedFifoEntry &entry) | |
| { | |
| auto &fu = fuState(FuKind::Compute); | |
| resetFu(fu); | |
| fu.destValid = entry.writeValid[0]; | |
| fu.destBank = MatrixBankKind::C; | |
| fu.destReg = entry.writeRegs[0]; | |
| DetailedCuteScoreboard::onComputeIssue(const DecodedFifoEntry &entry) | |
| { | |
| auto &fu = fuState(FuKind::Compute); | |
| resetFu(fu); | |
| fu.busy = true; | |
| fu.destValid = entry.writeValid[0]; | |
| fu.destBank = MatrixBankKind::C; | |
| fu.destReg = entry.writeRegs[0]; |
🤖 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/matrix/Scoreboard.cc` around lines 157 - 163,
DetailedCuteScoreboard::onComputeIssue currently resets fu then sets destination
fields but never marks the compute FU as busy; update the issue path to set
fu.busy = true after resetFu(fu) (and if you only want to mark occupancy when a
destination is actually written, set fu.busy = true conditionally when
entry.writeValid[0] is true) so the compute FU occupancy tracking (fu.busy)
remains consistent with other FUs.
Comment on lines
+95
to
+149
| bool canIssue(const DecodedFifoEntry &entry) const; | ||
| BlockReason blockReason(const DecodedFifoEntry &entry) const; | ||
| void onLoadIssue(const DecodedFifoEntry &entry); | ||
| void onLoadFinish(const DecodedFifoEntry &entry); | ||
| void onStoreIssue(const DecodedFifoEntry &entry); | ||
| void onStoreReadFinish(const DecodedFifoEntry &entry); | ||
| void onStoreWriteFinish(const DecodedFifoEntry &entry); | ||
| void onComputeIssue(const DecodedFifoEntry &entry); | ||
| void onComputeReadFinishA(const DecodedFifoEntry &entry); | ||
| void onComputeReadFinishB(const DecodedFifoEntry &entry); | ||
| void onComputeReadFinishC(const DecodedFifoEntry &entry); | ||
| void onComputeWriteFinishC(const DecodedFifoEntry &entry); | ||
| void onArithIssue(const DecodedFifoEntry &entry); | ||
| void onArithFinish(const DecodedFifoEntry &entry); | ||
| void onIssue(const DecodedFifoEntry &entry); | ||
| bool fuBusyForTest(FuKind fu) const; | ||
| bool regBusyForTest(uint8_t reg, MatrixBankKind bank) const; | ||
| unsigned pendingReadersForTest(uint8_t reg, MatrixBankKind bank) const; | ||
|
|
||
| private: | ||
| struct RegState | ||
| { | ||
| bool busy = false; | ||
| FuKind writer = FuKind::None; | ||
| unsigned pendingReaders = 0; | ||
| }; | ||
|
|
||
| std::vector<RegState> abRegs; | ||
| std::vector<RegState> cRegs; | ||
| std::array<FuState, static_cast<size_t>(FuKind::Count)> fuStates = {}; | ||
|
|
||
| static constexpr size_t SrcAIdx = 0; | ||
| static constexpr size_t SrcBIdx = 1; | ||
| static constexpr size_t SrcCIdx = 2; | ||
|
|
||
| FuState &fuState(FuKind fu); | ||
| const FuState &fuState(FuKind fu) const; | ||
| bool isFuBusy(FuKind fu) const; | ||
| RegState ®Status(uint8_t reg, MatrixBankKind bank); | ||
| const RegState ®Status(uint8_t reg, MatrixBankKind bank) const; | ||
| MatrixBankKind bankForSource(const DecodedFifoEntry &entry, size_t src_idx) const; | ||
| MatrixBankKind destBank(const DecodedFifoEntry &entry) const; | ||
| FuKind loadFu(const DecodedFifoEntry &entry) const; | ||
| bool sourceReady(const DecodedFifoEntry &entry, size_t src_idx) const; | ||
| bool destBusy(const DecodedFifoEntry &entry) const; | ||
| bool sourceHasPendingReaders(const DecodedFifoEntry &entry, size_t src_idx) const; | ||
| bool destHasPendingReaders(const DecodedFifoEntry &entry) const; | ||
| void resetSrc(SrcState &src); | ||
| void resetFu(FuState &fu); | ||
| void setupSrc(SrcState &src, MatrixBankKind bank, uint8_t reg); | ||
| void reserveDest(const DecodedFifoEntry &entry, FuKind writer); | ||
| void releaseDest(const DecodedFifoEntry &entry, FuKind writer); | ||
| void incrementPendingReader(uint8_t reg, MatrixBankKind bank); | ||
| void decrementPendingReader(uint8_t reg, MatrixBankKind bank); | ||
| void wakeupConsumers(uint8_t reg, MatrixBankKind bank, FuKind producer); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Normalize method/constant naming to repository conventions in this new scoreboard API.
Method names are not lower_snake_case, and compile-time constants (SrcAIdx, SrcBIdx, SrcCIdx) are not ALL_CAPS.
As per coding guidelines **/*.{cpp,cc,cxx,h,hpp,py}: Functions and methods should use lower_snake_case naming convention; Constants should use ALL_CAPS naming convention.
Also applies to: 126-128
🤖 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/matrix/Scoreboard.hh` around lines 95 - 149, The API uses camelCase and
mixed-case constants; rename all methods to lower_snake_case and compile-time
constants to ALL_CAPS to match project conventions: e.g. canIssue->can_issue,
blockReason->block_reason, onLoadIssue->on_load_issue,
onLoadFinish->on_load_finish, onStoreIssue->on_store_issue,
onStoreReadFinish->on_store_read_finish,
onStoreWriteFinish->on_store_write_finish, onComputeIssue->on_compute_issue,
onComputeReadFinishA/B/C->on_compute_read_finish_a/_b/_c,
onComputeWriteFinishC->on_compute_write_finish_c, onArithIssue->on_arith_issue,
onArithFinish->on_arith_finish, onIssue->on_issue,
fuBusyForTest->fu_busy_for_test, regBusyForTest->reg_busy_for_test,
pendingReadersForTest->pending_readers_for_test, fuState->fu_state,
isFuBusy->is_fu_busy, regStatus->reg_status, bankForSource->bank_for_source,
destBank->dest_bank, loadFu->load_fu, sourceReady->source_ready,
destBusy->dest_busy, sourceHasPendingReaders->source_has_pending_readers,
destHasPendingReaders->dest_has_pending_readers, resetSrc->reset_src,
resetFu->reset_fu, setupSrc->setup_src, reserveDest->reserve_dest,
releaseDest->release_dest, incrementPendingReader->increment_pending_reader,
decrementPendingReader->decrement_pending_reader,
wakeupConsumers->wakeup_consumers; and change SrcAIdx/SrcBIdx/SrcCIdx to
ALL_CAPS (e.g. SRC_A_IDX, SRC_B_IDX, SRC_C_IDX). Update all declarations and
corresponding definitions/usages to the new names.
Comment on lines
+76
to
+79
| DetailedCuteBackend::submit(const CuteRequest &req) | ||
| { | ||
| assert(canAccept(req)); | ||
| ++counters.fifoEnqueue; |
There was a problem hiding this comment.
Use runtime capacity enforcement in submit, not debug-only assert.
Line 78 only checks FIFO capacity in debug builds. In release builds this can silently enqueue beyond intended backpressure policy.
💡 Suggested fix
`#include` <cassert>
`#include` <utility>
+#include "base/logging.hh"
`#include` "base/trace.hh"
`#include` "debug/MatrixCuteTrace.hh"
`#include` "matrix/CUTETOP.hh"
void
DetailedCuteBackend::submit(const CuteRequest &req)
{
- assert(canAccept(req));
+ if (!canAccept(req)) {
+ panic("DetailedCuteBackend fifo overflow on submit [sn:%llu].",
+ static_cast<unsigned long long>(req.seq));
+ }
++counters.fifoEnqueue;Also applies to: 86-87
🤖 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/matrix/TaskController.cc` around lines 76 - 79, Replace the debug-only
assert in DetailedCuteBackend::submit with a runtime capacity check: call
canAccept(req) and if it returns false, perform the appropriate runtime
rejection/backpressure action (e.g. increment a rejection counter such as
counters.fifoRejected and return or throw the error code used by this backend)
instead of relying on assert; update both occurrences around
counters.fifoEnqueue (and the similar block at lines 86-87) so capacity is
enforced in release builds by explicit if (!canAccept(req)) { handle rejection }
before incrementing fifoEnqueue.
Comment on lines
+50
to
+56
| virtual bool canAccept(const CuteRequest &req) const = 0; | ||
| virtual void submit(const CuteRequest &req) = 0; | ||
| virtual bool hasWork() const = 0; | ||
| virtual void step() = 0; | ||
| virtual bool hasCompletion() const = 0; | ||
| virtual CuteCompletion popCompletion() = 0; | ||
| virtual bool hasArchitecturalState() const { return false; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Standardize interface/helper method names to lower_snake_case.
New API methods/functions in this header currently use camelCase, which conflicts with the repository naming rule for functions/methods.
As per coding guidelines **/*.{cpp,cc,cxx,h,hpp,py}: Functions and methods should use lower_snake_case naming convention.
Also applies to: 78-79, 123-129
🤖 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/matrix/TaskController.hh` around lines 50 - 56, The header exposes
methods using camelCase which violates the repo naming rule; rename the virtual
interface methods canAccept, submit, hasWork, step, hasCompletion,
popCompletion, and hasArchitecturalState to lower_snake_case (e.g., can_accept,
submit, has_work, step, has_completion, pop_completion,
has_architectural_state), then update all corresponding implementations,
overrides, callers, and tests to the new symbols; also apply the same renaming
to the other camelCase methods referenced around the file (the methods at the
other reported locations) so declarations and definitions stay in sync and
compilation/overrides remain correct.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
sada22222
changed the title
sada22222
changed the title
Change-Id: Ic4047011bb44b8aabc7b65df857b9cc36e1bf30c
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
added 4 commits
May 29, 2026 16:44
Change-Id: I5afb618667d0cca6e677b374b6c0837036a57e62
Change-Id: Ic8dd12afb56889633536f7ae619ded700dc6e67d
Reserve a matrix L2 fill table entry before issuing timing loads and hold the external source until matrix-register fill chunks retire. Track fill reservation, response, retire, and full-stall counters while making the fill chunk count configurable. Move MatrixRegResource into MRegFile so its arbitration state is owned with the register-file resource model. Change-Id: I649cbe3725a768fcf59197fae67fbe7a90fe4488
Remove the obsolete functional matrix memory adapter path and keep the detailed backend on the timing request flow. Update the O3 glue and matrix docs/build entries to match the timing-only interface. Change-Id: I3d56c5138e587f943a3f8bfeac9e8deb698a81ac
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/matrix/TaskController.cc (1)
283-319:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace debug-only
assert(false)with runtime panic.Line 317 guards an impossible case with
assert(false), which only triggers in debug builds. In release builds, ifMicroTaskKind::Countis somehow reached, the code silently falls through without handling the error, leading to undefined behavior.🛡️ Proposed fix
case MicroTaskKind::Release: releaseTask = task; break; case MicroTaskKind::Count: - assert(false && "dispatchTask called with unexpected compute/count task"); + panic("dispatchTask called with unexpected MicroTaskKind::Count [sn:%llu]", + static_cast<unsigned long long>(entry.request.seq)); } }🤖 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/matrix/TaskController.cc` around lines 283 - 319, dispatchTask currently uses assert(false) for the MicroTaskKind::Count branch which is optimized away in release builds; replace that assertion with a runtime panic/throw to abort with a clear error message. In DetailedCuteBackend::dispatchTask, change the MicroTaskKind::Count case to call the project's panic/fatal routine or throw a std::runtime_error containing contextual info (e.g., entry.request.seq and the unexpected microTaskKind value and backendStep) so the error is visible in release builds rather than silently undefined.
♻️ Duplicate comments (1)
src/matrix/TaskController.cc (1)
73-85:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse runtime capacity enforcement in
submit, not debug-onlyassert.Line 76 only checks FIFO capacity in debug builds. In release builds this can silently enqueue beyond intended backpressure policy and lead to buffer overflow or undefined behavior.
🛡️ Proposed fix
+#include "base/logging.hh" + namespace gem5 { namespace matrix { // Active request issue path: fifo, headReady, issueHead, dispatchTask.void DetailedCuteBackend::submit(const CuteRequest &req) { - assert(canAccept(req)); + if (!canAccept(req)) { + panic("DetailedCuteBackend fifo overflow on submit [sn:%llu].", + static_cast<unsigned long long>(req.seq)); + } ++counters.fifoEnqueue;🤖 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/matrix/TaskController.cc` around lines 73 - 85, The submit method currently relies on assert(canAccept(req)) which is disabled in release builds; replace this debug-only check with a runtime capacity enforcement: in DetailedCuteBackend::submit validate canAccept(req) at runtime and if it fails, increment the appropriate backpressure/reject counter (e.g., counters.fifoReject or similar) and return or propagate an error instead of enqueueing; only call fifo.enqueue(decodeCuteRequest(req)) when the runtime check passes. Ensure you reference and update the exact symbols: DetailedCuteBackend::submit, canAccept, counters (use an existing reject/backpressure counter or add one), fifo.enqueue, and decodeCuteRequest so the behavior is safe in release builds.
🧹 Nitpick comments (2)
src/matrix/TaskController.cc (1)
264-281: ⚡ Quick winClarify or remove empty if-block with misleading structure.
Lines 272-274 contain an if-check that guards nothing but a comment. The comment "ownership metadata no longer lives in MatrixRegFile" suggests code was removed, but the conditional remains. This is confusing for maintainability.
♻️ Proposed cleanup
If the check is no longer needed:
if (entry.isStore) { ++pendingStoreCount; } - if (entry.writeValid[0]) { - // ownership metadata no longer lives in MatrixRegFile - } - const auto kind = microTaskKindForEntry(entry);Or if the check is still relevant for future work, add a TODO:
if (entry.writeValid[0]) { - // ownership metadata no longer lives in MatrixRegFile + // TODO: Re-enable ownership tracking when MatrixRegFile supports 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 `@src/matrix/TaskController.cc` around lines 264 - 281, The if-block in DetailedCuteBackend::issueHead contains no body (it only holds a comment) making entry.writeValid[0] check misleading; either remove the entire if (entry.writeValid[0]) { ... } block or replace it with a brief TODO comment that documents why the check is kept (e.g., "TODO: preserve writeValid[0] check for future ownership metadata handling"); update the surrounding code in issueHead (including the comment currently inside that empty block) accordingly so the control flow is clear and no-empty conditional remains.src/cpu/o3/cpu.hh (1)
700-705: 💤 Low value
matrixBackendOwnersshould be inside the RISC-V guard for consistency.The
matrixBackendOwnersmember (line 705) is declared outside the#if THE_ISA_IS_RISCVblock while the related members (matrixShadowTokens,matrixTokenResetSeqs,matrixBackend) are inside. This creates an unused member on non-RISC-V builds.🔧 Suggested fix
`#if` THE_ISA_IS_RISCV std::vector<std::vector<RegVal>> matrixShadowTokens; std::vector<std::vector<InstSeqNum>> matrixTokenResetSeqs; std::unique_ptr<matrix::MatrixBackend> matrixBackend; -#endif std::unordered_map<InstSeqNum, ThreadID> matrixBackendOwners; +#endif🤖 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/cpu.hh` around lines 700 - 705, Move the matrixBackendOwners member declaration inside the existing RISC-V preprocessor guard so it matches matrixShadowTokens, matrixTokenResetSeqs, and matrixBackend; specifically, place the std::unordered_map<InstSeqNum, ThreadID> matrixBackendOwners; declaration within the `#if` THE_ISA_IS_RISCV / `#endif` block alongside matrixShadowTokens, matrixTokenResetSeqs and matrixBackend to avoid defining it on non-RISC-V builds.
🤖 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.
Outside diff comments:
In `@src/matrix/TaskController.cc`:
- Around line 283-319: dispatchTask currently uses assert(false) for the
MicroTaskKind::Count branch which is optimized away in release builds; replace
that assertion with a runtime panic/throw to abort with a clear error message.
In DetailedCuteBackend::dispatchTask, change the MicroTaskKind::Count case to
call the project's panic/fatal routine or throw a std::runtime_error containing
contextual info (e.g., entry.request.seq and the unexpected microTaskKind value
and backendStep) so the error is visible in release builds rather than silently
undefined.
---
Duplicate comments:
In `@src/matrix/TaskController.cc`:
- Around line 73-85: The submit method currently relies on
assert(canAccept(req)) which is disabled in release builds; replace this
debug-only check with a runtime capacity enforcement: in
DetailedCuteBackend::submit validate canAccept(req) at runtime and if it fails,
increment the appropriate backpressure/reject counter (e.g., counters.fifoReject
or similar) and return or propagate an error instead of enqueueing; only call
fifo.enqueue(decodeCuteRequest(req)) when the runtime check passes. Ensure you
reference and update the exact symbols: DetailedCuteBackend::submit, canAccept,
counters (use an existing reject/backpressure counter or add one), fifo.enqueue,
and decodeCuteRequest so the behavior is safe in release builds.
---
Nitpick comments:
In `@src/cpu/o3/cpu.hh`:
- Around line 700-705: Move the matrixBackendOwners member declaration inside
the existing RISC-V preprocessor guard so it matches matrixShadowTokens,
matrixTokenResetSeqs, and matrixBackend; specifically, place the
std::unordered_map<InstSeqNum, ThreadID> matrixBackendOwners; declaration within
the `#if` THE_ISA_IS_RISCV / `#endif` block alongside matrixShadowTokens,
matrixTokenResetSeqs and matrixBackend to avoid defining it on non-RISC-V
builds.
In `@src/matrix/TaskController.cc`:
- Around line 264-281: The if-block in DetailedCuteBackend::issueHead contains
no body (it only holds a comment) making entry.writeValid[0] check misleading;
either remove the entire if (entry.writeValid[0]) { ... } block or replace it
with a brief TODO comment that documents why the check is kept (e.g., "TODO:
preserve writeValid[0] check for future ownership metadata handling"); update
the surrounding code in issueHead (including the comment currently inside that
empty block) accordingly so the control flow is clear and no-empty conditional
remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1bd3c113-df6c-49d1-9222-0bafee21f378
📒 Files selected for processing (15)
docs/Gem5_Docs/xsai/Matrix_Readme.mdsrc/cpu/o3/BaseO3CPU.pysrc/cpu/o3/cpu.ccsrc/cpu/o3/cpu.hhsrc/matrix/CUTETOP.ccsrc/matrix/CUTETOP.hhsrc/matrix/LocalMMUModel.ccsrc/matrix/LocalMMUModel.hhsrc/matrix/MRegFile.ccsrc/matrix/MRegFile.hhsrc/matrix/MemoryLoader.ccsrc/matrix/MemoryLoader.hhsrc/matrix/SConscriptsrc/matrix/TaskController.ccsrc/matrix/TaskController.hh
💤 Files with no reviewable changes (1)
- src/matrix/MemoryLoader.cc
✅ Files skipped from review due to trivial changes (1)
- docs/Gem5_Docs/xsai/Matrix_Readme.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/matrix/SConscript
- src/matrix/TaskController.hh
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
task_plan.md (1)
77-77: 💤 Low valueConsider adding blank lines around tables for better Markdown formatting.
The markdownlint tool suggests adding blank lines before and after tables at lines 77 and 90 to improve readability and comply with MD058 (blanks-around-tables).
✨ Suggested formatting improvement
Add a blank line before line 77:
| `plan.md` 是派生执行计划,不是 matrix 计划真源 | `Plans.md` 仍是阶段/status/DoD 真源 | | `humanize-gen-plan` 使用 `findings.md` 作为主草稿输入 | `findings.md` 已聚合需求、事实、风险、验证和 review gate,适合作为生成计划的稳定草稿 | | 先用 `/tmp/gem5-humanize-phase4a-plan.md` 通过 IO 校验,再更新现有 `plan.md` | Humanize 校验脚本要求输出文件不存在,而项目根目录已有用户指定的 `plan.md` | | 下一新会话从 `plan.md` 执行,但先恢复 planning 文件上下文 | 避免 Humanize/RLCR 实现时把旧 `32/256` 口径或 cache 范围误带入代码 | | 新会话先修 O3 MMA `payload.op -> CuteRequest.op` 透传,再回到 MatrixReg/MTE 主线 | 避免 `mfmacc_s_h` 在真实 O3/AMU backend request 中被 `makeMma()` 默认 `0x0c` 覆盖 | + ## 遇到的错误 | 错误 | 尝试次数 | 解决方案 |Add a blank line after line 94:
| 错误 | 尝试次数 | 解决方案 | |------|---------|----------| | 未发现脚本级错误 | 0 | 当前仅做文档抽取与文件创建 | | `validate-gen-plan-io.sh --output plan.md` 会因输出已存在而不适合直接使用 | 1 | 改用 `/tmp/gem5-humanize-phase4a-plan.md` 做 IO 校验,随后按用户要求更新现有 `plan.md` | + ## 备注Also applies to: 90-90
🤖 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 `@task_plan.md` at line 77, The Markdown table starting with the header row "| 决策 | 理由 |" needs blank lines before and after it (and likewise for the other table instance) to satisfy markdownlint MD058; edit the document to insert a single empty line immediately above the table header and another empty line immediately below the table's closing row so both tables are separated by blank lines from surrounding paragraphs.Source: Linters/SAST tools
findings.md (2)
62-73: ⚡ Quick winTechnical decision table is helpful but could be expanded.
The decision table provides good rationale for key choices, but consider adding a "Status" or "Date" column to track:
- When each decision was made
- Whether it's still active or has been superseded
- Any plans to revisit the decision
This would make the table more useful for long-term maintenance.
🤖 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 `@findings.md` around lines 62 - 73, The decision table in findings.md lacks metadata to track lifecycle; add two new columns "Status" and "Date" to the table header and update every row (the existing rows under the header that list decisions like "后续优先把 Phase 4A 作为 RLCR 执行对象", "plan.md 的验收标准按 AC 拆分功能..." etc.) to include current status values (e.g., Active/Superseded) and the decision date; ensure the header separator row and every existing data row are extended to match the new columns so the markdown table renders correctly and consider adding a short legend or note below the table describing allowed status values.
100-107: ⚡ Quick winTrack the known O3→CuteRequest MMA opcode metadata loss as a formal issue.
The document correctly identifies that
toCuteRequest()doesn't copypayload.optoout.opfor MMA instructions, causingmfmacc_s_hopcode metadata to be recorded as0x0cinstead of0x04. While the suggestion to "先用 focused check 或 TDD 修" (fix with focused check or TDD first) in the next session is good, this bug should also be:
- Filed as a tracked issue/TODO in your issue tracker
- Tagged with appropriate priority (seems like a correctness issue even if functional path isn't affected yet)
- Assigned an owner if known
This ensures the issue doesn't get lost if the next session focuses on other work 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 `@findings.md` around lines 100 - 107, The MMA path in toCuteRequest() (used when building CuteRequest via CuteRequest::makeMma()) currently leaves out.op at the default 0x0c so payload.op (e.g., mfmacc_s_h which should be 0x04) is not propagated; update the MMA branch in toCuteRequest() to assign out.op = payload.op, add a focused unit/integration test that constructs an MMA decode (mfmacc_s_h) and asserts out.op matches payload.op, and file a tracked issue/TODO in the repository referencing toCuteRequest(), CuteRequest::makeMma(), payload.op, out.op and mfmacc_s_h with a correctness priority and an owner so it is triaged if not fixed immediately.docs/Gem5_Docs/xsai/Matrix_Params.md (2)
160-169: ⚡ Quick winVerification priority list is clear but could benefit from explicit action items.
The "最需要 RTL 核对的参数差异" (Most needed RTL verification differences) section provides a good prioritized list of discrepancies, but each item would be more actionable if it included:
- Who is responsible for verification
- Target timeline
- Link to tracking issue/task
This would transform the list from documentation into an executable plan.
🤖 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 `@docs/Gem5_Docs/xsai/Matrix_Params.md` around lines 160 - 169, Convert each numbered verification item into an actionable task by adding (1) an owner (e.g., an engineer or team) responsible for verifying the parameter, (2) a target due date or sprint (e.g., YYYY‑MM‑DD or Sprint X), and (3) a link to a tracking issue or ticket; for example for items referencing MatrixRegFile A/B/C vs. ABMatrixRegCount/NumAbRegs/NumCRegs, MatrixMn/TensorMn, matrixL2FillCChunksPerBeat/CMatrixRegEntryByteSize, Response.ready, source release behavior, bank drain bandwidth, acceptedInputBeats+6+cdcWriteCycles, CMatrixReg parity/conflict, LocalMMU backend step width, and release/sync token semantics—add the owner, timeline, and tracking link per item and include a one‑line expected verification step (e.g., "measure RTL visible registers vs. gem5 config" or "confirm C fill chunk granularity with 64B responses") so each discrepancy is immediately actionable and traceable.
11-18: ⚡ Quick winConsider using relative or environment-variable-based paths instead of hardcoded absolute paths.
The document contains multiple hardcoded absolute paths to RTL files (e.g.,
/nfs/home/hujun/workspace/xsai/xsai-env/XSAI/CUTE/src/main/scala/...). These paths will not work for other developers or on different systems, making the documentation less portable and harder to maintain.Consider one of these approaches:
- Use relative paths from a documented repository root (e.g.,
$XSAI_ROOT/CUTE/src/main/scala/...)- Define environment variables in a setup section
- Use generic placeholders like
<RTL_ROOT>/CUTE/src/main/scala/...🤖 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 `@docs/Gem5_Docs/xsai/Matrix_Params.md` around lines 11 - 18, The table uses hardcoded absolute paths; replace them with repository-relative or environment-variable-backed placeholders (e.g., $XSAI_ROOT or <RTL_ROOT>) and add a short "Setup" note in the docs that defines the variable. Update every entry referencing artifacts such as MatrixInstClass/MatrixRouteKind, needAmuCtrlCandidate, mtilem/mtilen/mtilek, msyncregreset, mrelease, macquire, the LSU/MMA rows and their file references (TaskController.scala, AMemoryLoader.scala, CUTEParameters.hh, Fence.scala, MatrixRegTensor_* etc.) to use the new placeholder/relative form instead of absolute /nfs/... paths, and ensure the doc header documents how to set $XSAI_ROOT or the chosen placeholder.docs/exec-plans/active/matrix-rtl-aligned-modeling-draft.md (1)
1-51: ⚡ Quick winConsider whether this draft should be kept alongside the full plan.
This file (
matrix-rtl-aligned-modeling-draft.md) appears to be a planning draft, whilematrix-rtl-aligned-modeling-plan.mdin the same directory is a comprehensive, detailed plan covering similar topics.Consider:
- If the draft served its purpose and the full plan is complete, the draft could be moved to an archive or removed to avoid confusion
- If the draft still has value (e.g., historical context), add a note at the top referencing the full plan
- Ensure there are no important details in the draft that didn't make it into the full plan
🤖 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 `@docs/exec-plans/active/matrix-rtl-aligned-modeling-draft.md` around lines 1 - 51, The draft file matrix-rtl-aligned-modeling-draft.md duplicates material in matrix-rtl-aligned-modeling-plan.md and risks confusion; either archive/delete the draft or explicitly mark it as historical by adding a top-note linking to matrix-rtl-aligned-modeling-plan.md, and scan the draft for any unique details not present in matrix-rtl-aligned-modeling-plan.md (merge any missing content into the full plan) before removing or moving the draft to an archive folder and updating any docs index or TOC references.plan.md (2)
273-273: ⚡ Quick winHardcoded user-specific path in verification command.
Similar to the earlier comment on lines 55-64, line 273 contains a user-specific absolute path
/nfs/home/hujun/workspace/xsai/.... Consider documenting this as an example path or using an environment variable like$XSAI_FIRMWARE_PATHto make the plan more portable.🤖 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 `@plan.md` at line 273, The verification command embeds a hardcoded user-specific absolute path (`/nfs/home/hujun/workspace/xsai/...`) inside the gem5 invocation (`./build/RISCV/gem5.opt --outdir=... configs/example/se.py -c .../gemm_precomp ...`); update the plan to avoid the hardcoded path by replacing it with an example placeholder or environment variable (e.g. `$XSAI_FIRMWARE_PATH` or `XSAI_FIRMWARE_PATH`) and document that the user must set that variable, ensuring the command references the variable instead of the absolute `/nfs/home/hujun/...` path.
55-64: ⚡ Quick winHardcoded user-specific absolute paths may hinder collaboration.
The absolute paths on lines 55 and 59-64 reference
/nfs/home/hujun/which are user-specific. Consider either:
- Using relative paths from the repository root
- Adding a note that these paths are examples and should be adjusted per developer environment
- Moving these references to a separate local configuration file that isn't committed
This will make it easier for other team members to use this plan document.
🤖 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 `@plan.md` around lines 55 - 64, The plan.md contains hardcoded, user-specific absolute paths pointing to example source files (see references like AMemoryLoader.scala, BMemoryLoader.scala, CMemoryLoader.scala, ABMatrixReg.scala, CMatrixReg.scala, CUTEParameters.scala) which hinder portability; update the document to replace absolute /nfs/home/... entries with relative repository-root-relative paths or placeholder variables (e.g., <REPO_ROOT>/path/to/...) and add a short note that these are examples and must be adjusted per developer environment, or move these references into a non-committed local config snippet referenced from plan.md so collaborators can override their own paths.
🤖 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 `@docs/blog/posts/first-post.md`:
- Line 2: The frontmatter 'date' in docs/blog/posts/first-post.md is set to
"2025-05-14" but other PR docs reference 2026; confirm the intended publication
date and update the frontmatter key `date:` in docs/blog/posts/first-post.md to
the correct value (e.g., "2026-05-14" if it should be 2026) so it matches the
project's timeline and other documentation.
In `@progress.md`:
- Line 103: The progress note shows "AC-1 到 AC-9" but plan.md only defines AC-1
through AC-8; confirm whether an acceptance criterion was removed or AC-9 is
missing, then either (a) update the string "AC-1 到 AC-9" in progress.md to "AC-1
到 AC-8" if AC-9 was removed, or (b) restore/add the missing AC-9 entry into
plan.md with the correct acceptance criteria and keep progress.md as-is; ensure
numbering and references to "AC-1"..."AC-9" are consistent across both files.
---
Nitpick comments:
In `@docs/exec-plans/active/matrix-rtl-aligned-modeling-draft.md`:
- Around line 1-51: The draft file matrix-rtl-aligned-modeling-draft.md
duplicates material in matrix-rtl-aligned-modeling-plan.md and risks confusion;
either archive/delete the draft or explicitly mark it as historical by adding a
top-note linking to matrix-rtl-aligned-modeling-plan.md, and scan the draft for
any unique details not present in matrix-rtl-aligned-modeling-plan.md (merge any
missing content into the full plan) before removing or moving the draft to an
archive folder and updating any docs index or TOC references.
In `@docs/Gem5_Docs/xsai/Matrix_Params.md`:
- Around line 160-169: Convert each numbered verification item into an
actionable task by adding (1) an owner (e.g., an engineer or team) responsible
for verifying the parameter, (2) a target due date or sprint (e.g., YYYY‑MM‑DD
or Sprint X), and (3) a link to a tracking issue or ticket; for example for
items referencing MatrixRegFile A/B/C vs. ABMatrixRegCount/NumAbRegs/NumCRegs,
MatrixMn/TensorMn, matrixL2FillCChunksPerBeat/CMatrixRegEntryByteSize,
Response.ready, source release behavior, bank drain bandwidth,
acceptedInputBeats+6+cdcWriteCycles, CMatrixReg parity/conflict, LocalMMU
backend step width, and release/sync token semantics—add the owner, timeline,
and tracking link per item and include a one‑line expected verification step
(e.g., "measure RTL visible registers vs. gem5 config" or "confirm C fill chunk
granularity with 64B responses") so each discrepancy is immediately actionable
and traceable.
- Around line 11-18: The table uses hardcoded absolute paths; replace them with
repository-relative or environment-variable-backed placeholders (e.g.,
$XSAI_ROOT or <RTL_ROOT>) and add a short "Setup" note in the docs that defines
the variable. Update every entry referencing artifacts such as
MatrixInstClass/MatrixRouteKind, needAmuCtrlCandidate, mtilem/mtilen/mtilek,
msyncregreset, mrelease, macquire, the LSU/MMA rows and their file references
(TaskController.scala, AMemoryLoader.scala, CUTEParameters.hh, Fence.scala,
MatrixRegTensor_* etc.) to use the new placeholder/relative form instead of
absolute /nfs/... paths, and ensure the doc header documents how to set
$XSAI_ROOT or the chosen placeholder.
In `@findings.md`:
- Around line 62-73: The decision table in findings.md lacks metadata to track
lifecycle; add two new columns "Status" and "Date" to the table header and
update every row (the existing rows under the header that list decisions like
"后续优先把 Phase 4A 作为 RLCR 执行对象", "plan.md 的验收标准按 AC 拆分功能..." etc.) to include
current status values (e.g., Active/Superseded) and the decision date; ensure
the header separator row and every existing data row are extended to match the
new columns so the markdown table renders correctly and consider adding a short
legend or note below the table describing allowed status values.
- Around line 100-107: The MMA path in toCuteRequest() (used when building
CuteRequest via CuteRequest::makeMma()) currently leaves out.op at the default
0x0c so payload.op (e.g., mfmacc_s_h which should be 0x04) is not propagated;
update the MMA branch in toCuteRequest() to assign out.op = payload.op, add a
focused unit/integration test that constructs an MMA decode (mfmacc_s_h) and
asserts out.op matches payload.op, and file a tracked issue/TODO in the
repository referencing toCuteRequest(), CuteRequest::makeMma(), payload.op,
out.op and mfmacc_s_h with a correctness priority and an owner so it is triaged
if not fixed immediately.
In `@plan.md`:
- Line 273: The verification command embeds a hardcoded user-specific absolute
path (`/nfs/home/hujun/workspace/xsai/...`) inside the gem5 invocation
(`./build/RISCV/gem5.opt --outdir=... configs/example/se.py -c .../gemm_precomp
...`); update the plan to avoid the hardcoded path by replacing it with an
example placeholder or environment variable (e.g. `$XSAI_FIRMWARE_PATH` or
`XSAI_FIRMWARE_PATH`) and document that the user must set that variable,
ensuring the command references the variable instead of the absolute
`/nfs/home/hujun/...` path.
- Around line 55-64: The plan.md contains hardcoded, user-specific absolute
paths pointing to example source files (see references like AMemoryLoader.scala,
BMemoryLoader.scala, CMemoryLoader.scala, ABMatrixReg.scala, CMatrixReg.scala,
CUTEParameters.scala) which hinder portability; update the document to replace
absolute /nfs/home/... entries with relative repository-root-relative paths or
placeholder variables (e.g., <REPO_ROOT>/path/to/...) and add a short note that
these are examples and must be adjusted per developer environment, or move these
references into a non-committed local config snippet referenced from plan.md so
collaborators can override their own paths.
In `@task_plan.md`:
- Line 77: The Markdown table starting with the header row "| 决策 | 理由 |" needs
blank lines before and after it (and likewise for the other table instance) to
satisfy markdownlint MD058; edit the document to insert a single empty line
immediately above the table header and another empty line immediately below the
table's closing row so both tables are separated by blank lines from surrounding
paragraphs.
🪄 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: 045c91bf-becb-42ad-917b-7b9daa30a37f
📒 Files selected for processing (35)
PR_README.mdcute_detailed_design/Plans.mdcute_detailed_design/handoff.mdcute_detailed_design/implementation_notes.mdcute_detailed_design/matrix_core_design.mdcute_detailed_design/matrix_diff_vs_rtl.mdcute_detailed_design/matrix_o3_README.mdcute_detailed_design/mls-tlb-replay-fix-plan.mdcute_detailed_design/official-README.mdcute_detailed_design/verify_report.mddocs/Gem5_Docs/frontend/phr_design.mddocs/Gem5_Docs/xsai/Matrix_Design.mddocs/Gem5_Docs/xsai/Matrix_Params.mddocs/Gem5_Docs/xsai/Matrix_RTL_Modeling_Review.mddocs/blog/index.mddocs/blog/posts/first-post.mddocs/exec-plans/active/kmhv3-run-commands.mddocs/exec-plans/active/matrix-l2-adapter-fill-table.mddocs/exec-plans/active/matrix-reliable-ipc-baseline.mddocs/exec-plans/active/matrix-rtl-aligned-modeling-draft.mddocs/exec-plans/active/matrix-rtl-aligned-modeling-plan.mddocs/tools/alignToRTL/rvv-simple/handoff.mddocs/tools/alignToRTL/rvv-simple/implementation_notes.mddocs/tools/alignToRTL/rvv-simple/phase0.mddocs/tools/alignToRTL/rvv-simple/verify_report.mdfindings.mdplan.mdprogress.mdsrc/matrix/local_mmu_model.test.ccsrc/matrix/matrix_reg_resource.test.ccsrc/matrix/scoreboard.test.ccsrc/matrix/timing_load_plan.test.ccsrc/matrix/timing_store_backend.test.ccsrc/matrix/timing_store_plan.test.cctask_plan.md
✅ Files skipped from review due to trivial changes (9)
- docs/exec-plans/active/matrix-reliable-ipc-baseline.md
- cute_detailed_design/matrix_o3_README.md
- cute_detailed_design/official-README.md
- docs/blog/index.md
- docs/exec-plans/active/kmhv3-run-commands.md
- docs/Gem5_Docs/xsai/Matrix_Design.md
- docs/tools/alignToRTL/rvv-simple/phase0.md
- docs/tools/alignToRTL/rvv-simple/handoff.md
- docs/tools/alignToRTL/rvv-simple/implementation_notes.md
| @@ -0,0 +1,22 @@ | |||
| --- | |||
| date: 2025-05-14 | |||
There was a problem hiding this comment.
Verify the publication date is correct.
The blog post is dated 2025-05-14, but the PR objectives and other documentation files reference activities in 2026 (e.g., "2026-06-03", "2026-06-04" in matrix-l2-adapter-fill-table.md). This suggests either:
- The blog post date is a placeholder that should be updated
- There's a year typo (should be
2026-05-14)
Please verify the intended publication date is correct.
🤖 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 `@docs/blog/posts/first-post.md` at line 2, The frontmatter 'date' in
docs/blog/posts/first-post.md is set to "2025-05-14" but other PR docs reference
2026; confirm the intended publication date and update the frontmatter key
`date:` in docs/blog/posts/first-post.md to the correct value (e.g.,
"2026-05-14" if it should be 2026) so it matches the project's timeline and
other documentation.
| - 执行的操作： | ||
| - 以 `findings.md` 为主草稿，结合 `Plans.md`、`handoff.md`、`matrix_diff_vs_rtl.md`、`verify_report.md` 和 `reviews/matrix_review_standard.md` 重新生成 `plan.md`。 | ||
| - 新 `plan.md` 补强了： | ||
| - AC-1 到 AC-9 |
There was a problem hiding this comment.
Inconsistency in acceptance criteria count.
Line 103 mentions "AC-1 到 AC-9" (AC-1 to AC-9), but plan.md only defines AC-1 through AC-8. Either:
- An acceptance criterion was removed but this progress log wasn't updated, or
- There's a missing AC-9 in
plan.md
Please verify which is correct and update accordingly for consistency.
🤖 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 `@progress.md` at line 103, The progress note shows "AC-1 到 AC-9" but plan.md
only defines AC-1 through AC-8; confirm whether an acceptance criterion was
removed or AC-9 is missing, then either (a) update the string "AC-1 到 AC-9" in
progress.md to "AC-1 到 AC-8" if AC-9 was removed, or (b) restore/add the missing
AC-9 entry into plan.md with the correct acceptance criteria and keep
progress.md as-is; ensure numbering and references to "AC-1"..."AC-9" are
consistent across both files.
7ed06da to
0b63f41
Compare
Wait for staged macquire tokens at commit and relax mrelease readiness to pending stores. Change-Id: I0d4ea56edb7a9b5530c8e4d8458ae44337eecee0 (cherry picked from commit 7ed06da)
Change-Id: I492c69cef2b9e57b6ca7a9a6ed280778c367e7ac
Change-Id: Ia701d8720700b68d15e322fecd6edc266b31a8a8
Change-Id: Id3c839f34db401b6076ade1c2a892a84e32eddab
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Change-Id: I0290766970c563ad540da0e1d3a90f03557213df
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Add default-on O3 matrix switches for backend service, matrix memory port connection, and MLS queue handling. Disabled matrix service paths avoid coupling with scalar/vector execution and fail fast where those disabled services are reached. Document matrix enablement and the RVV simple build/run flow. Change-Id: Ie38af898d2ebfae1da560686a29dfc1c6ca84558
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This PR adds a more complete matrix execution path for XSAI GEMM in gem5. It connects decoded matrix instructions through the O3 pipeline, commits matrix requests into a backend-visible AMU buffer path, and models a more detailed CUTE backend for matrix load/store, MMA, release, and completion.
The goal is not full RTL cycle equivalence yet. The goal is to provide a runnable, observable, and extensible matrix backend model that can pass the current SE GEMM smoke workload while making the remaining RTL alignment gaps explicit.
What Changed
Add O3 matrix instruction plumbing:
MatrixAmuBufferrequest ownership and completion flowAdd detailed CUTE backend model:
MatrixCuteTraceAdd matrix functional memory access for SE smoke:
Gem5MatrixMemoryAdapterloadTile()/storeTile()readMatrixElem<T>()/writeMatrixElem<T>()Move the detailed matrix backend document to:
docs/Gem5_Docs/xsai/Matrix_Readme.mdScope and Limitations
This PR intentionally keeps several parts as timing/resource abstractions rather than full RTL-equivalent implementations:
MRegFileis still logical whole-tensor functional storage, not a physical 8-bank SRAM model.mtilem/mtilen/mtilekstill use the renameable misc-register path.Validation
Built gem5:
Ran SE GEMM smoke with matrix KMHV3 scheduler:
Result:
Review Notes
Recommended review focus:
MatrixAmuBufferapproximation boundary versus RTLAMUCtrlBuffer.mls_unit.cc.Summary by CodeRabbit
New Features
Updates
Documentation