[dyn-seq] Trim dma_bd SSA comments per review

Apply Andrej's review suggestions (AIE.td arith dependency, dma_bd offset/len
docs, buildWithConstants) and give the sibling comments the same concise
treatment: drop the internal cross-references and upstream-op name-drops, keep
the load-bearing "why".

Co-Authored-By: Claude <noreply@anthropic.com>

Author: hunhoffe

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

@@ -26,12 +26,9 @@ def AIE_Dialect : Dialect {
   }];
   let useDefaultTypePrinterParser = 1;
   let useDefaultAttributePrinterParser = 1;
-  // Several AIE constructs materialize arith ops via the builder API (e.g. the
-  // Python wrappers in dialects/aie.py build arith.constant for runtime scalar
-  // operands, and AIEX lowerings emit arith into runtime_sequence bodies). MLIR
-  // requires dependent dialects to be pre-loaded before ops referencing them may
-  // be built, so declaring the dependency at dialect level is the safe placement.
+  // Python wrappers in dialects/aie.py and AIEX lowerings emit arith.constant
+  // and other operations, so this dialect depends on arith being available.
   let dependentDialects = ["::mlir::arith::ArithDialect"];
 }
 
-#endif // AIE_BASE
+#endif // AIE_BASE

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

@@ -982,12 +982,10 @@ def AIE_DMABDOp: AIE_Op<"dma_bd", [AttrSizedOperandSegments]> {
 
   let arguments = (
     ins AnyRankedOrUnrankedMemRef:$buffer,
-        // offset/len are denominated in element width (not bytes). Like the
-        // sizes/strides DynamicIndexList below (and aiex.npu.dma_memcpy_nd's
-        // static_offsets), a compile-time constant lands in the
-        // $static_offset/$static_len attribute while a runtime value becomes the
-        // $offset/$len SSA operand. Absent operand AND attr means offset 0 /
-        // full-buffer len.
+        // offset/len are denominated in element width (not bytes).
+        // Can be given as a static attribute or a runtime SSA value. If neither
+        // are given, we assume zero for the offset and the memref's full buffer
+        // length for the length.
         Optional<I32>:$offset,
         Optional<I32>:$len,
         OptionalAttr<AIEI32Attr>:$static_offset,
@@ -1032,20 +1030,17 @@ def AIE_DMABDOp: AIE_Op<"dma_bd", [AttrSizedOperandSegments]> {
     // constant offset/len to the static_offset/static_len attributes (no
     // arith.constant materialized) and decomposes a BDDimLayoutArrayAttr
     // (outermost-first) into all-static static_sizes / static_strides arrays (no
-    // dynamic operands). Keeps every historical int-argument producer call site
-    // source-compatible.
+    // dynamic operands).
     static void buildWithConstants(
         ::mlir::OpBuilder &builder, ::mlir::OperationState &state,
         ::mlir::Value buffer, int32_t offset, int32_t len,
         BDDimLayoutArrayAttr dims, BDPadLayoutArrayAttr padDims,
         PacketInfoAttr packet);
 
     // Mixed static/dynamic builder: splits offset/len and each sizes/strides
-    // entry (a constant attribute or an SSA Value) into the operand + static
-    // attribute pair via the upstream dispatchIndexOpFoldResults helper, exactly
-    // like tensor.extract_slice. An empty sizes/strides list leaves the
-    // corresponding static array unset so it elides; a null offset/len leaves
-    // both its operand and attribute unset.
+    // entry into the operand + static attribute pair via the upstream
+    // dispatchIndexOpFoldResults helper. An empty sizes/strides list leaves the
+    // corresponding static array unset so it elides.
     static void buildMixed(
         ::mlir::OpBuilder &builder, ::mlir::OperationState &state,
         ::mlir::Value buffer, ::mlir::OpFoldResult offset,
@@ -1061,12 +1056,9 @@ def AIE_DMABDOp: AIE_Op<"dma_bd", [AttrSizedOperandSegments]> {
       return dataLayout.getTypeSize(bufferType.getElementType());
     }
 
-    // The offset/len as compile-time constants. A constant lives in the
-    // static_offset/static_len attribute; if instead a runtime operand is
-    // present, fold it (a constant-valued operand still resolves). Returns
-    // nullopt when neither is present or the operand is a genuine runtime value
-    // (callers on the static lowering path treat that as an error via
-    // getFoldedDimensions' sibling diagnostics).
+    // The offset/len as a compile-time constant: the static_offset/static_len
+    // attribute, or a constant-valued operand folded down. Returns nullopt for a
+    // genuine runtime value.
     std::optional<int32_t> getConstantOffset() {
       if (auto attr = getStaticOffset())
         return static_cast<int32_t>(*attr);
@@ -1084,9 +1076,9 @@ def AIE_DMABDOp: AIE_Op<"dma_bd", [AttrSizedOperandSegments]> {
       return std::nullopt;
     }
 
-    // True when the BD carries an explicit length (a runtime operand or the
-    // static_len attribute). Distinct from getConstantLen(), which is nullopt
-    // for a runtime len even though a length IS present.
+    // True when the BD carries an explicit length (runtime operand or
+    // static_len attribute), even a runtime one for which getConstantLen() is
+    // nullopt.
     bool hasLen() { return getLen() || getStaticLen().has_value(); }
     bool hasOffset() { return getOffset() || getStaticOffset().has_value(); }
 

lib/Dialect/AIE/IR/AIEDialect.cpp

@@ -2033,11 +2033,8 @@ BufferOp DMABDOp::getBufferOp() {
 
 // Parse/print hooks for the custom<DynamicScalar>($operand, $static_attr)
 // directive: a single scalar that is either an SSA value (runtime, %v) or a
-// compile-time integer constant (folded into the attribute). Mirrors
-// custom<DynamicIndexList> but for one scalar rather than a list, so a constant
-// never materializes an operand. The generated code binds the Optional<I32>
-// operand as std::optional<UnresolvedOperand> and the OptionalAttr<AIEI32Attr>
-// as IntegerAttr, exactly like upstream transform.tune AlternativesOp.
+// compile-time integer constant (folded into the attribute), so a constant
+// never materializes an operand. The scalar analog of custom<DynamicIndexList>.
 static ParseResult
 parseDynamicScalar(OpAsmParser &parser,
                    std::optional<OpAsmParser::UnresolvedOperand> &operand,
@@ -2068,9 +2065,7 @@ static void printDynamicScalar(OpAsmPrinter &printer, Operation *,
 }
 
 // Split a scalar OpFoldResult into an operand (runtime value) or an i32
-// attribute (compile-time constant), mirroring how sizes/strides split a
-// constant into static_* and a runtime value into an operand. An absent (null)
-// OpFoldResult leaves both unset.
+// attribute (compile-time constant). A null OpFoldResult leaves both unset.
 static void splitScalarOfr(mlir::OpBuilder &builder, mlir::OpFoldResult ofr,
                            mlir::Value &operand, mlir::IntegerAttr &attr) {
   operand = nullptr;
@@ -2092,15 +2087,15 @@ void DMABDOp::buildMixed(mlir::OpBuilder &builder, mlir::OperationState &state,
                          llvm::ArrayRef<mlir::OpFoldResult> sizes,
                          llvm::ArrayRef<mlir::OpFoldResult> strides,
                          BDPadLayoutArrayAttr padDims, PacketInfoAttr packet) {
-  // Split each mixed list into its dynamic operands + static array with the
-  // ShapedType::kDynamic sentinel, exactly as tensor.extract_slice does.
+  // Split each mixed list into its dynamic operands + static array (with the
+  // ShapedType::kDynamic sentinel for the runtime entries).
   llvm::SmallVector<int64_t> staticSizes, staticStrides;
   llvm::SmallVector<mlir::Value> dynSizes, dynStrides;
   mlir::dispatchIndexOpFoldResults(sizes, dynSizes, staticSizes);
   mlir::dispatchIndexOpFoldResults(strides, dynStrides, staticStrides);
 
   // Leave the static arrays unset when there is no ND layout so they elide
-  // from the printed form (a plain linear transfer has no sizes/strides).
+  // from the printed form.
   DenseI64ArrayAttr staticSizesAttr =
       staticSizes.empty() ? DenseI64ArrayAttr{}
                           : builder.getDenseI64ArrayAttr(staticSizes);
@@ -2137,13 +2132,13 @@ void DMABDOp::buildWithConstants(mlir::OpBuilder &builder,
                                  BDPadLayoutArrayAttr padDims,
                                  PacketInfoAttr packet) {
   // Constant offset/len flow to the static_offset/static_len attributes (no
-  // arith.constant materialized). Use i32 to match the AIEI32Attr type so
-  // splitScalarOfr's cast stays sign-exact for negative offsets.
+  // arith.constant materialized). i32 matches AIEI32Attr, keeping the cast
+  // sign-exact for negative offsets.
   mlir::OpFoldResult offsetOfr = builder.getI32IntegerAttr(offset);
   mlir::OpFoldResult lenOfr = builder.getI32IntegerAttr(len);
 
   // Turn the outermost-first BDDimLayoutArrayAttr into all-constant
-  // OpFoldResults and let buildMixed handle the decomposition.
+  // OpFoldResults for buildMixed.
   llvm::SmallVector<mlir::OpFoldResult> sizes, strides;
   if (dims) {
     for (BDDimLayoutAttr d : dims) {
@@ -2261,11 +2256,9 @@ LogicalResult DMABDOp::verify() {
   if (std::optional<int32_t> nextBdId = getNextBdId();
       nextBdId.has_value() && static_cast<uint32_t>(*nextBdId) >= maxBds)
     return emitOpError("nextBdId attribute exceeds max: ") << maxBds - 1;
-  // Guard the mixed static/dynamic invariant before any consumer counts
-  // sentinels against operands: the number of ShapedType::kDynamic entries in
-  // each static array must equal the number of dynamic operands, and sizes and
-  // strides must agree in rank. Skips the crash in getMixedValues when the two
-  // are inconsistent (llvm #179401).
+  // The dynamic-operand count must match the kDynamic-sentinel count in each
+  // static array, and sizes/strides must agree in rank. Checked before any
+  // consumer calls getMixedValues, which crashes on a mismatch (llvm #179401).
   llvm::ArrayRef<int64_t> staticSizes =
       getStaticSizes().value_or(llvm::ArrayRef<int64_t>{});
   llvm::ArrayRef<int64_t> staticStrides =
@@ -2282,9 +2275,7 @@ LogicalResult DMABDOp::verify() {
            << ")";
 
   // Fold the mixed sizes/strides to a constant BDDimLayoutAttr list for
-  // verification. A runtime size/stride yields nullopt + a diagnostic; on the
-  // static verification path that is an error. `dims` mirrors the historical
-  // std::optional<ArrayRef> shape so the checks below are unchanged.
+  // verification; a runtime size/stride yields nullopt plus a diagnostic.
   std::optional<llvm::SmallVector<BDDimLayoutAttr>> dims;
   if (!getMixedSizes().empty()) {
     dims = getConstantDimensions();
@@ -2403,10 +2394,10 @@ LogicalResult DMABDOp::verify() {
       return emitOpError("Packet ID field can only hold 5 bits.");
   }
 
+  // A runtime len operand or the static_len attribute both count as having a
+  // length here.
   if (!hasLen() && !getBuffer().getType().hasStaticShape())
     return emitOpError() << "buffer with dynamic shape requires static length.";
-  // Either a runtime operand or the static_len attribute satisfies the "has a
-  // length" requirement.
 
   if (getBurstLength() != 0 && !parentTile.isShimNOCTile())
     return emitOpError("Burst length is only supported in Shim NOC tiles that "
@@ -2511,12 +2502,11 @@ struct LinearizeContiguousBDTransfer : public mlir::OpRewritePattern<DMABDOp> {
     if (!bufferIsExternal && !parentIsShim && !inShimDMA)
       return mlir::failure();
 
-    // Only when ND dimensions are present and all-constant. Runtime-valued
-    // sizes can't be folded/linearized; decline silently (no diagnostic) so
-    // valid IR with runtime sizes doesn't produce a spurious error.
+    // Only ND dimensions that are present and all-constant can be linearized;
+    // decline silently on runtime-valued sizes/strides so valid dynamic IR
+    // doesn't get a spurious diagnostic.
     if (op.getMixedSizes().empty())
       return mlir::failure();
-    // Check all sizes/strides are constants before attempting to fold.
     for (mlir::OpFoldResult s : op.getMixedSizes())
       if (!mlir::getConstantIntValue(s))
         return mlir::failure();
@@ -2548,17 +2538,13 @@ struct LinearizeContiguousBDTransfer : public mlir::OpRewritePattern<DMABDOp> {
       return mlir::failure();
     int32_t len = static_cast<int32_t>(product);
 
-    // Rewrite to a linear BD: set len as the static_len attribute (a constant,
-    // so no arith.constant operand) and clear the sizes/strides (both operands
-    // and static arrays). All other attributes (offset, packet, burst_length,
-    // bd_id, etc.) are preserved automatically.
+    // Rewrite to a linear BD: len as the static_len attribute, sizes/strides
+    // cleared. Other attributes are preserved.
     rewriter.modifyOpInPlace(op, [&]() {
       op.getLenMutable().clear();
       op.setStaticLen(len);
       op.getSizesMutable().clear();
       op.getStridesMutable().clear();
-      // Drop the static arrays entirely so the linear BD prints without any
-      // sizes/strides clause (the attributes are optional).
       op.setStaticSizes(std::nullopt);
       op.setStaticStrides(std::nullopt);
     });
@@ -2568,10 +2554,7 @@ struct LinearizeContiguousBDTransfer : public mlir::OpRewritePattern<DMABDOp> {
 } // namespace
 
 // Canonicalization pattern for DMABDOp: fold a constant offset/len/size/stride
-// operand back into the corresponding static attribute (removing the operand),
-// via the upstream foldDynamicIndexList helper for the lists and
-// getConstantIntValue for the scalars. Mirrors the constant-argument folding
-// tensor.extract_slice performs for its offsets/sizes/strides.
+// operand back into the corresponding static_* attribute, removing the operand.
 namespace {
 struct FoldConstantBDDimList : public mlir::OpRewritePattern<DMABDOp> {
   using OpRewritePattern::OpRewritePattern;
@@ -2606,8 +2589,7 @@ struct FoldConstantBDDimList : public mlir::OpRewritePattern<DMABDOp> {
     rewriter.modifyOpInPlace(op, [&]() {
       op.getSizesMutable().assign(dynSizes);
       op.getStridesMutable().assign(dynStrides);
-      // Leave the static arrays unset (not empty) when there is no ND layout so
-      // they elide from the printed form (a plain linear transfer).
+      // Leave the static arrays unset when there is no ND layout so they elide.
       if (staticSizes.empty())
         op.removeStaticSizesAttr();
       else

python/dialects/aie.py

@@ -109,14 +109,9 @@ def _as_i32(v):
 
 def _split_i32_scalar(v):
     """Split a dma_bd offset/len argument into (operand, static_attr): a Python
-    int becomes a compile-time attribute (no arith.constant materialized), an SSA
-    Value becomes the runtime operand, and None leaves both unset. Mirrors the
-    operand-vs-attribute split sizes/strides already use.
-
-    Contrast with ``_as_i32``: that helper always emits an ``arith.constant``
-    for integer inputs, which is correct for scalar ops whose operand IS an SSA
-    i32. Here a constant lands in the static_offset/static_len attribute instead,
-    so no constant op is materialized."""
+    int becomes the static attribute, an SSA Value becomes the runtime operand,
+    and None leaves both unset. Unlike ``_as_i32``, an int stays an attribute
+    rather than being materialized as an arith.constant operand."""
     if v is None:
         return None, None
     if isinstance(v, (int, np.integer)):
@@ -154,8 +149,7 @@ def dma_bd(
     offset_operand, static_offset = _split_i32_scalar(offset)
     len_operand, static_len = _split_i32_scalar(transfer_len)
 
-    # Leave the static arrays unset (not empty) when there is no ND layout so
-    # the optional attributes elide from the printed op.
+    # Leave the static arrays unset when there is no ND layout so they elide.
     return _DMABDOp(
         buffer,
         sizes=dyn_sizes,

python/iron/dataflow/tile_dma.py

@@ -95,11 +95,9 @@ class Bd:
     # ``(pkt_type, pkt_id)``.  Pairs with a :class:`PacketFlow` that uses
     # the same ``pkt_id`` so the routing fabric dispatches correctly.
     packet: tuple[int, int] | None = None
-    # Multi-dimensional (strided) access pattern, given as parallel lists
-    # matching ``aie.dma_bd``'s ``sizes``/``strides`` operands (outermost
-    # dimension first). Each entry may be a Python int (constant) or an SSA
-    # ``Value`` (runtime). Empty lists (default) emit a plain contiguous
-    # transfer. ``sizes`` and ``strides`` must have the same length.
+    # Strided access pattern, outermost dimension first; each entry is a
+    # constant int or a runtime Value. Empty (default) emits a contiguous
+    # transfer. sizes and strides must have equal length.
     sizes: list = field(default_factory=list)
     strides: list = field(default_factory=list)
 
@@ -244,8 +242,6 @@ def _body(block):
                     with block[bd_block_idx[bd_pos]]:
                         for acq in bd.acquires:
                             acq.emit()
-                        # dma_bd takes sizes/strides directly (matches
-                        # npu.dma_memcpy_nd's DynamicIndexList operands).
                         bd_kwargs: dict[str, Any] = dict(
                             sizes=bd.sizes, strides=bd.strides
                         )