Enable dynamic lock acquisition for ObjectFIFO acquire/release in cores#3308
Merged
Conversation
andrej
force-pushed
the
objfifo-locks-dynamic
branch
from
e9be3d4 to
901b0a8
Compare
Collaborator
|
I'm glad to see this Andrej, it's a big improvement on code size! |
Collaborator
|
Out of curiosity, does this address either of the two recently opened issues? |
andrej
force-pushed
the
objfifo-locks-dynamic
branch
from
9a94506 to
671d0ef
Compare
andrej
commented
Jul 10, 2026
andrej
left a comment
andrej
left a comment
Collaborator
Author
There was a problem hiding this comment.
Can we add a custom parser that allows immediate values in the IR instead of forcing arith.constant everywhere to reduce the size of the diff / having to rewrite all tehse tests?
andrej
requested review from
abisca,
denolf,
erwei-xilinx,
fifield,
hunhoffe,
jackl-xilinx,
jgmelber,
pvasireddy-amd and
stephenneuendorffer
as code owners
hunhoffe
approved these changes
Jul 13, 2026
hunhoffe
left a comment
hunhoffe
left a comment
Collaborator
There was a problem hiding this comment.
Looks good! This is a good time to do this type of change, since we're already doing a lot of type conversions to SSA
| Worker( | ||
| conv1x1_fn, | ||
| fn_args=[wts_buf_00.cons(), of_act_l3l2.cons(), of_act_2_3_5.prod(), conv1], | ||
| stack_size=0x1000, |
Collaborator
There was a problem hiding this comment.
Interesting! Are you sure the changes to this design are necessary? I'm surprised it was passing before this!
Collaborator
Author
There was a problem hiding this comment.
Yes, they definitely are. This was hard to debug, along with another missized buffer. Keep in mind we are now keeping lock count bookkeeping data in memory. If that gets overridden by a buffer override, it causes the whole design to misbehave/timeout. Previously, it might just have overridden other data which might have passed some of our fuzzy output tests.
Make aie.use_lock carry its lock count as an i32 SSA operand instead of an optional integer attribute, and emit runtime-computed lock counts in the dynamic (loop-preserving) objectFifo lowering via a per-(fifo,port) "held" counter. Passes that need a compile-time constant read it back through UseLockOp::getConstantValue(). The static loop-unrolling path (dynamic-objFifos=false) is left in place and still emits compile-time constant lock counts, so static and dynamic lock lowering coexist on this branch. Compiler code only; the corresponding test updates follow in the next commit.
Update lock-bearing goldens for the new i32 SSA aie.use_lock value form, add dynamic_runtime_lock_* tests covering the runtime "held"-counter lock bookkeeping, add a bad-input test for a non-constant use_lock value where a constant is required, and drop the dynamic_cyclostatic_* goldens that exercised the removed static peel/held-before-loop lock accounting.
Emit the per-(fifo,port) runtime held-element counters as single-element memref.alloca (tagged aie.held_counter) so acquire/release bookkeeping is plain load/store, then run mem2reg in-pass to promote them to SSA values. Hard-error if any counter survives promotion. Regenerate the dynamic objectFifo lock CHECK lines accordingly.
Co-authored-by: André Rösti <androsti@amd.com>
andrej
force-pushed
the
objfifo-locks-dynamic
branch
from
5578f86 to
25ca535
Compare
hunhoffe
added a commit
that referenced
this pull request
Jul 13, 2026
Integrates PR #3308 (dynamic lock acquisition for ObjectFIFO): - All test files: use_lock uses new SSA i32 value operand form (arith.constant + %c) - All test files: dma_bd retains this PR's SSA operand format (offset=/len=/sizes=/strides=) - bad_bd_assignments.mlir: includes #3308's new nextBdId overflow test - dynamic_cyclostatic_* test files removed (deleted upstream by #3308) - New dma_channel_pinning/ tests and dynamic_runtime_lock_* tests from #3308 Co-Authored-By: Claude <noreply@anthropic.com>
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
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.
The --dynamic-objFifos option -- now the default -- so far made the buffer selection dynamic in the ObjectFIFO. This means the lowering injects some code into the IR which resolves which buffer to read from/write to at runtime instead of reasoning about this at compile-time and hard-coding the buffers.
This PR does the same for locks, allowing acquiring and releasing objects within arbitrary control flow paths, while a small runtime component keeps track of the number of objects that the core holds. This means we keep a runtime-computed value for the lock acquire/release counts in a register or in memory on the core.
To enable this, we have to refactor the IR to use SSA values for lock counts rather than static attributes -- this is the reason this PR touches many tests. Existing passes that rely on constant values for lock acquire/release emit arith.constant (or trace the SSA value back to a defining arith.constant and error if the value is not constant).