Repository navigation
feat(rocjitsu): Add a pipeline latch and IssueQueue for clocked components - #14308
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
atgutier
left a comment
atgutier
left a comment
There was a problem hiding this comment.
Looks good to me, just a nit about pragma once and an ask to remove some complexity for now.
8d9cfb4 to
20a1215
Compare
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
It would be useful to make a few small type and timing details explicit:
- In
emulation/rocjitsu/lib/simdojo/include/simdojo/components/pipe_latch.h:94, renamevisibletovisible_cycleso comparisons and updates read as timestamps rather than a boolean property. - At
pipe_latch.h:99, usestd::numeric_limits<uint64_t>::max()(as simdojo'sTICK_MAXdoes) and document that the initial value means no write yet. - In
emulation/rocjitsu/tests/simdojo_pipe_latch_test.cpp:101, spell outstd::unique_ptr<int> valueto make the move-only result type explicit and follow the style guide's rule for function-returnauto. - At
simdojo_pipe_latch_test.cpp:73, spell outlatency + 1instead of introducing the undefined shorthandL.
Cycle-level models built from Clocked components need registers between their pipeline stages. Add simdojo::PipeLatch: a producer stage writes a value during a cycle, the owning component clocks the latch on the clock edge, and the consumer stage takes the value in the next cycle. Nothing written in a cycle is visible before the edge, so the stages of one cycle may run in any order. A latch is a single register: it applies no back-pressure and adds no latency beyond the edge. It is plain state driven by its owning component and schedules no events. Multi-cycle units, such as execution pipelines and memory round trips, need entries to wait longer than one edge. Add simdojo::IssueQueue: a producer pushes an entry with the first cycle it may leave, and the consumer pops the oldest entry once that cycle arrives. Entries leave in order. A fixed-latency pipeline pushes with the current cycle plus its latency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
20a1215 to
19c929e
Compare
amd-arosa
changed the title
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
We should update the documentation to match the simplified primitives:
- The PR Test Plan and Test Result still describe nine
PipeLatchTestcases, including latency and back-pressure. The current suites have five latch tests and four queue tests. Please describe those cases and identify which revision the reported full-suite results cover. - Add the two headers to
emulation/rocjitsu/docs/simdojo.md's file overview and component library, with a short example of running stages beforeclock(). Its Simulation Time section currently says all timestamps and latencies useTick; clarify that this queue instead uses owner-maintained cycle indices. This gives callers one consistent reference for composing the new primitives.
| /// any order. | ||
| /// | ||
| /// @tparam T Entry type. It must be movable. | ||
| template <typename T> class IssueQueue { |
There was a problem hiding this comment.
I think we should name this for its timed FIFO behavior, for example DelayQueue. It releases entries in insertion order using a fixed ready-cycle stamp; it does not select among ready instructions or track later changes to a wave's readiness. A behavior-based name would make its role clearer for execution delays and memory returns, and distinguish it from the CU's scheduling policy before the follow-up PRs adopt it.
| PipeLatch<int> latch; | ||
| latch.write(1); | ||
| latch.clock(); | ||
| latch.clock(); |
There was a problem hiding this comment.
It would be good to add an unread-value replacement test: write(1); clock(); write(2); clock(); should make take() return 2. The current tests cover a bubble replacing an unread value, but consume the old value before testing replacement with another value. A scratch mutation that retained the old value when both slots were populated still passed all five latch tests. The implementation is correct; this test would protect its documented overwrite behavior.
| return !entries_.empty() && entries_.front().ready_cycle <= cycle; | ||
| } | ||
|
|
||
| /// @brief Remove and return the oldest entry. |
There was a problem hiding this comment.
It would help to state here that pop() does not check readiness and callers must check ready(cycle) before a timed departure. The class describes that protocol, but this method's only listed precondition is non-emptiness, and pop() will also return an entry before its ready cycle. Making the caller's responsibility explicit beside the operation would clarify the timing contract without changing the API.
| IssueQueue<std::unique_ptr<int>> queue; | ||
| queue.push(std::make_unique<int>(3), 1); | ||
| ASSERT_TRUE(queue.ready(1)); | ||
| auto value = queue.pop(); |
There was a problem hiding this comment.
Please spell out std::unique_ptr<int> value = queue.pop(); here. This makes the move-only result type visible at the assertion and follows docs/style.md's rule against function-return auto when the type is not apparent on the same line.
TheRock Submodule Bump ActivityNewest first
|
Adds
simdojo::PipeLatch, a pipeline register for models built fromClockedcomponents. It is the first of four PRs that give a clocked CU driver the pieces it needs to decide which waves issue each cycle; this one is standalone and touches only simdojo.Motivation
to add PipeLatch and IssueQueue
Technical Details
PipeLatch<T>is a single register. The producer stagewrite()s during a cycle, the owning component callsclock()on the clock edge after every stage has run, and the consumer stagetake()s the value in the next cycle. Nothing written in a cycle is visible before the edge, so the stages of one cycle may run in any order. The latch applies no back-pressure and adds no latency beyond the edge;clock()replaces whatever the consumer did not take.IssueQueue<T>holds entries that wait, in order, until each one's ready cycle. A producerpush()es an entry with the first cycle it may leave, and the consumerpop()s the oldest entry onceready(cycle)says that cycle has arrived. An entry never overtakes an older one, even if its own ready cycle is earlier. A fixed-latency pipeline pushes with the current cycle plus its latency.Both are header-only, plain state driven by their owning component, and schedule no events:
lib/simdojo/include/simdojo/components/pipe_latch.hlib/simdojo/include/simdojo/components/issue_queue.hIssue Tracking
#14308
Test Plan
New
PipeLatchTest(9 cases) covers latency, one-per-cycle throughput, back-pressure from a stalled consumer, bubbles closing behind a stalled head, move-only entries, and a three-stage clocked pipeline run with its stages ticked in both orders.Test Result
Built in Release with ROCm 7.3 clang, with the whole stack applied on
develop.PipeLatchTest: 9/9 pass.ctestpasses 8907 of 8922. All 15 failures are unrelated to this stack:Gfx1250ExecutionTest.TensorDma*cases segfault identically ondevelopwithout these changes.HsaTestcases fail because the test binary cannot link against this host's HSA runtime (hsa_amd_queue_createis missing).SystemGraphTest.DaemonStartsSelectedNodecannot create its runtime directory under/tmpon this host.pre-commit run --from-ref origin/develop --to-ref HEADis clean.Submission Checklist