[mlir][amdgpu] Add amdgpu.make_dma_descriptor #169407
Conversation
🐧 Linux x64 Test Results
|
|
|
||
| let summary = "TODO"; | ||
| let description = [{ | ||
| TODO |
| custom<DynamicIndexList>($atomic_barrier_dynamic_indices, $atomic_barrier_static_indices) | ||
| `:` type($atomic_barrier_address) `)`)? | ||
| ( `iterate` $global_increment^ `,` $lds_increment `,` $iteration_count )? | ||
| attr-dict `:` qualified(type($base)) `to` type(results) |
| printer << staticSize.getValue(); | ||
| } else { |
| found in TensorLoadToLDSOp and TensorStoreFromLDSOp in the rocdl dialect. | ||
| }]; |
There was a problem hiding this comment.
Can you also add an mlir example?
| int64_t staticVal; | ||
| if (parser.parseOptionalInteger(staticVal).has_value()) { |
There was a problem hiding this comment.
nit/optional: you could push the definition inside the if, since it not used elsewhere
| int64_t staticVal; | |
| if (parser.parseOptionalInteger(staticVal).has_value()) { | |
| if (int64_t staticVal; parser.parseOptionalInteger(staticVal).has_value()) { |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| found in TensorLoadToLDSOp and TensorStoreFromLDSOp in the rocdl dialect. | ||
| }]; |
| 2D and 3D tensors may be iterated over by setting $global_increment, $lds_increment, and $iteration_count. | ||
| $global_increment determines how much to increment the starting global memory address per iteration in units of the $base's element type. | ||
| $lds_increment determines how much to increment the starting LDS address per iteration in units of the $base's element type. | ||
| $iterate_count determines how many times to iterate. |
There was a problem hiding this comment.
also here: can you add a few mlir examples?
There was a problem hiding this comment.
I added a few examples, I will ask for clarification on iteration.
| OptionalParseResult parseResult = parser.parseOptionalInteger(staticVal); | ||
| if (parseResult.has_value()) { |
There was a problem hiding this comment.
nit: why do we need this as a local variable, instead of using in the if condition like before?
| //===----------------------------------------------------------------------===// | ||
|
|
||
| LogicalResult MakeDmaDescriptorOp::verify() { | ||
| if (getGlobalStaticStrides()->back() != 1) { |
There was a problem hiding this comment.
does something else already guarantee there's at least one element?
Co-authored-by: Jakub Kuderski <[email protected]>
|
|
||
| // TODO: Define a custom printer, parser to avoid space between $src/%dst and indices. | ||
| let assemblyFormat = [{ | ||
| $src custom<DynamicIndexList>($src_indices, $src_indices_const) `,` |
There was a problem hiding this comment.
I assume this is out of date
| Arguments<(ins | ||
| AMDGPU_TDMBaseType: $base, | ||
| Variadic<Index>: $global_dynamic_sizes, | ||
| OptionalAttr<DenseI64ArrayAttr>: $global_static_sizes, |
There was a problem hiding this comment.
Existing examples of a similar pattern - subview, for example, don't use an optional here
(The static indices array must have a fixed length and use -1 to refer to dynamic dimensions)
| OptionalAttr<IndexAttr>: $every_const, | ||
| Optional<AnyMemRef>: $atomic_barrier_address, | ||
| Variadic<Index>: $atomic_barrier_dynamic_indices, | ||
| OptionalAttr<DenseI64ArrayAttr>: $atomic_barrier_static_indices, |
There was a problem hiding this comment.
The atomic barrier part is more of a pure memref operation, and should probably just take Variadic<Index> (it's selecting a pointer, unlike all the other bits, which are doing size/strides/offsets subview-style)
| Optional<Index>: $pad, | ||
| OptionalAttr<IndexAttr>: $pad_const, | ||
| Optional<Index>: $every, | ||
| OptionalAttr<IndexAttr>: $every_const, |
There was a problem hiding this comment.
This can just be an optional index - doesn't need attribute splitting
| OptionalAttr<DenseI64ArrayAttr>: $shared_static_sizes, | ||
| Optional<Index>: $pad, | ||
| OptionalAttr<IndexAttr>: $pad_const, | ||
| Optional<Index>: $every, |
There was a problem hiding this comment.
Let's call this padEvery? Or something else more descriptive?
| } // namespace | ||
|
|
||
| static ParseResult | ||
| parseDynamicIndex(OpAsmParser &parser, |
There was a problem hiding this comment.
Shouldn't need this
For the non-vararg cases, we can use an argument.
For the vararg cases (sizes, strides, etc.) we'll want to use the same machinery memref.subview does.
| if (getGlobalStaticStrides()->back() != 1) { | ||
| return emitOpError("strides for the innermost dimension must be 1."); | ||
| } | ||
| return success(); |
There was a problem hiding this comment.
Missing validation conditions: Number of strides == number of sizes == number of offsets, and the global and LDS tiles have the same dimensions
| attr-dict `:` qualified(type($base)) `->` type(results) | ||
| }]; | ||
|
|
||
| let hasVerifier = 1; |
There was a problem hiding this comment.
We'll want the OpFoldResult helpers for combining the static/dynamic parts, I claim.
There was a problem hiding this comment.
Good point here but let's leave that for the next PR which will add the lowering.
3 participants
No description provided.