Skip to content

Commit 29468c2

Browse files
committed
[objectfifo] Address review: tighten endpoints, reuse infrastructure
Every DMA endpoint now states the direction it moves data in, shim ends included. The direction was previously recovered from the endpoint's position in its flow, through a map built for the purpose, because a shim endpoint had no pool and so dropped its role; saying it outright deletes the map. A core endpoint selecting only part of an object is rejected. The lowering hands a core whole buffers, so such an endpoint would have silently received the wrong memref; emitting the memref.subview it needs is what would let cores take part in a join or distribute. iter_count is recorded only on the MemTile endpoint that honors it, and rejected elsewhere, instead of being carried onto endpoints that ignore it. Reuses existing infrastructure the review pointed at: TileOp::getOrCreate for tile lookup, SymbolUserMap for deciding which pools are still referenced, and DenseSet for channel occupancy, which is all that was ever stored. The DMA program search and the channel scan lose their per-op-type triplication. Buffer byte size moves onto the pool op. FIXMEs mark the three places the review identified as belonging elsewhere: buffer placement, packet ID assignment, and the repeat_count/iter_count tangle.
1 parent 8899a75 commit 29468c2

30 files changed

Lines changed: 737 additions & 470 deletions

docs/design/objfifo-refactor-plan.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ Target IR at each stage is documented in `docs/design/objfifo-stages/`. Read
1010

1111
- `test/objectFifo-stateful-transform` stays green: 159 pass, 1 XFAIL.
1212
- Existing tests are unchanged, or changed only mechanically (RUN lines, op
13-
spellings). A behavioural diff in an expected output means the commit is wrong.
14-
- New behaviour ships with new tests in the same commit.
13+
spellings). A behavioral diff in an expected output means the commit is wrong.
14+
- New behavior ships with new tests in the same commit.
1515
- Comments follow `docs/skills` / the code-commenting skill: no history, no
1616
before/after framing, no restating the adjacent line.
1717

@@ -163,7 +163,7 @@ The unroll hint and the alloca-to-SSA promotion belong to this pass: the hint
163163
preserves fifo-level information the lowering discards, and the allocas exist
164164
only to reach SSA form.
165165

166-
New behaviour, with tests:
166+
New behavior, with tests:
167167

168168
- an acquire on an endpoint selecting several segments takes one lock per
169169
segment, all with the same delta, before the object is handed over;
@@ -200,7 +200,7 @@ an aie.objectfifo.acquire" verifier and
200200
`test/objectFifo-stateful-transform/subview_escape_via_iter_args.mlir` all go.
201201

202202
Mechanical but wide: most objectFifo tests use `subview.access`. Land it last so
203-
it never blocks a behavioural commit.
203+
it never blocks a behavioral commit.
204204

205205
---
206206

docs/design/objfifo-stages/00-model.mlir

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,8 @@
140140
// which gives three shapes:
141141
//
142142
// one segment spanning the object -> the buffer itself
143-
// one segment that is a strict slice -> a memref.subview at its offset
143+
// one segment that is a strict slice -> a memref.subview at its offset,
144+
// not emitted yet and so rejected
144145
// every segment of a partitioned object -> the buffer itself, after N acquires
145146
//
146147
// A core endpoint's selection is fixed, so every acquire on it yields the same
@@ -226,7 +227,7 @@
226227
//
227228
// - a pool's segments do not overlap and cover the element type exactly
228229
// - each of a pool's segments has one filling endpoint and one draining
229-
// endpoint. A filler may be absent when the lock initialisers mark the
230+
// endpoint. A filler may be absent when the lock initializers mark the
230231
// objects as starting full
231232
// - every flow reaches a destination
232233
//
@@ -239,16 +240,18 @@
239240
//
240241
// IMPLEMENTATION NOTES
241242
//
242-
// 1. A core endpoint whose segments do not span the whole buffer needs a
243-
// memref.subview at the segment offset before the object is handed over. Do
244-
// not assume elementType == buffer type on a core endpoint.
243+
// 1. A core endpoint whose segments do not span the whole buffer would need a
244+
// memref.subview at the segment offset before the object is handed over.
245+
// That is not emitted yet, so the verifier rejects such an endpoint rather
246+
// than handing over the whole buffer; implementing it is what would let
247+
// cores take part in a join or distribute.
245248
//
246249
// 2. A multi-segment core acquire takes one lock per segment before the object
247250
// may be touched. The delta computation is unchanged; it is applied to each.
248251
//
249252
// 3. Pre-filled fifos desugar during split: the initial contents become
250-
// initialisers on the aie.buffer ops, and "N objects start full" becomes the
251-
// pool's lock initialisers.
253+
// initializers on the aie.buffer ops, and "N objects start full" becomes the
254+
// pool's lock initializers.
252255
//
253256
//===----------------------------------------------------------------------===//
254257
//

include/aie/Dialect/AIE/IR/AIEOps.td

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2284,12 +2284,12 @@ def AIE_ObjectFifoPoolOp
22842284
OptionalAttr<FlatSymbolRefArrayAttr>:$buffers,
22852285
OptionalAttr<ObjectFifoSegmentArrayAttr>:$segments,
22862286
OptionalAttr<FlatSymbolRefArrayAttr>:$locks,
2287-
// Times the pool is cycled through per outer iteration; lock initialisers
2287+
// Times the pool is cycled through per outer iteration; lock initializers
22882288
// scale with it.
22892289
OptionalAttr<AIEI32Attr>:$repeatCount,
22902290
// Outer iterations the DMA chain runs for.
22912291
OptionalAttr<AIEI32Attr>:$iterCount,
2292-
// No locks are created for this pool; its actors are unsynchronised.
2292+
// No locks are created for this pool; its actors are unsynchronized.
22932293
DefaultValuedAttr<BoolAttr, "false">:$disableSynchronization,
22942294
// Name of the objectFifo this pool holds the objects of.
22952295
OptionalAttr<StrAttr>:$fifoName, InitValuesArrayAttr:$initValues);
@@ -2307,6 +2307,9 @@ def AIE_ObjectFifoPoolOp
23072307
/// Byte-agnostic element count of one object, taken from the element type.
23082308
int64_t getObjectSize();
23092309

2310+
/// Size of one object in bytes, under the closest data layout.
2311+
int64_t getObjectSizeInBytes();
2312+
23102313
/// One entry per way the object is divided; a pool with no `segments` is
23112314
/// one whole object.
23122315
llvm::SmallVector<ObjectFifoSegmentAttr> getSegmentAttrs();
@@ -2383,7 +2386,7 @@ def AIE_ObjectFifoDmaEndpointOp
23832386
}];
23842387

23852388
let arguments = (ins SymbolNameAttr:$sym_name, Index:$tile,
2386-
OptionalAttr<ObjectFifoRole>:$role, OptionalAttr<FlatSymbolRefAttr>:$pool,
2389+
ObjectFifoRole:$role, OptionalAttr<FlatSymbolRefAttr>:$pool,
23872390
OptionalAttr<DenseI32ArrayAttr>:$segments,
23882391
OptionalAttr<ObjectFifoChannelAttr>:$channel,
23892392
// Channel the user demands, claimed before any is auto-assigned.
@@ -2408,7 +2411,7 @@ def AIE_ObjectFifoDmaEndpointOp
24082411
OptionalAttr<StrAttr>:$fifoName);
24092412

24102413
let assemblyFormat = [{
2411-
$sym_name `(` $tile `)` ($role^ $pool)? attr-dict
2414+
$sym_name `(` $tile `)` $role (`of` $pool^)? attr-dict
24122415
}];
24132416

24142417
let hasVerifier = 1;

include/aie/Dialect/AIE/Transforms/AIEDMAChannelAnalysis.h

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,10 @@ namespace xilinx::AIE {
1818
/// Which DMA channels of each tile are already spoken for, so that a channel is
1919
/// handed out at most once across everything that programs one.
2020
class DMAChannelAnalysis {
21-
mlir::DenseMap<std::tuple<mlir::Value, DMAChannelDir, int>, int>
22-
channelsPerTile;
23-
mlir::DenseMap<std::tuple<mlir::Value, DMAChannelDir, int>, int>
24-
aieStreamsPerTile;
21+
/// A channel or stream port is either spoken for or free, so membership is
22+
/// the whole state.
23+
mlir::DenseSet<std::tuple<mlir::Value, DMAChannelDir, int>> usedChannels;
24+
mlir::DenseSet<std::tuple<mlir::Value, DMAChannelDir, int>> usedStreams;
2525

2626
public:
2727
DMAChannelAnalysis(DeviceOp &device);
@@ -33,7 +33,8 @@ class DMAChannelAnalysis {
3333
bool requiresAdjacentTileAccessChannels);
3434

3535
/// Claim `channel` for (`tile`, `dir`) so first-free assignment cannot take
36-
/// it. Returns -1 when the channel is out of range or already claimed.
36+
/// it. Returns the channel, or -1 when it is out of range or already
37+
/// claimed; the caller reports, since it knows which endpoint asked.
3738
int reservePinnedChannel(TileLike tile, DMAChannelDir dir, int channel);
3839

3940
/// Claim a raw stream port, reporting on `tile` when it is already taken.

0 commit comments

Comments
 (0)