Added test operation to replace linalg.generic and linalg.copy. Also added
test operation for scf.if and scf.for. Adapted tests in all test cases using
buffer-deallocation, buffer-hoisting, buffer-loop-hoisting,
promote-buffers-to-stack, TestBufferPlacement and replaced those operations.
These operations were added to decouple the corresponding dialects from
the tests.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Could you expand the description to add the rationale? It is mostly focussed on what but I don't know why
mlir/test/lib/Dialect/Test/TestOps.td | ||
---|---|---|
1798–1809 | Do we really need it to be as powerful as linalg.generic to test conversion of a tensor-based op to a memref-based op. def TensorBasedOp : TEST_Op<"tensor-based", [AttrSizedOperandSegments, SingleBlockImplicitTerminator<"YieldOp">]> { let description = [{ <some incredibly useful comment>c. }]; let arguments = (ins Variadic<AnyMemRef>:$inputs); let results = (outs Variadic<AnyMemRef>:$results); let regions = (region AnyRegion:$region); <use assemblyForm here> } def BufferBasedOp : TEST_Op<"buffer-based", [AttrSizedOperandSegments, SingleBlockImplicitTerminator<"YieldOp">]> { let description = [{ <some incredibly useful comment>c. }]; let arguments = (ins Variadic<AnyRankedTensor>:$inputs); let results = (outs Variadic<AnyRankedTensor>:$results); let regions = (region AnyRegion:$region); <use assemblyForm here> } be sufficient? | |
mlir/test/lib/Transforms/TestBufferPlacement.cpp | ||
192 ↗ | (On Diff #300265) | nit: populateTestTensorToBufferConversionPattern |
@jpienaar +1 for that. I can also provide some context. BufferPlacementPreparation pass was using linalg dialect to test conversion of tensor-based ops to buffer-based ones (like HLO->LHLO). The pass was added a while ago and since then Linalg tensor-to-buffer conversion became more advanced and complicated. It became confusing to have two locations in mlir codebase where linalg was converted. It has led to submitting code and, more importantly, tests to the test directory instead of linalg.
mlir/test/lib/Dialect/Test/TestOps.td | ||
---|---|---|
1798–1809 | Yes, the idea is sufficient and we implemented it like you suggested. I think you reversed the names, but in general this issue should be addressed. |
Very well, you're almost there.
mlir/test/Transforms/promote-buffers-to-stack.mlir | ||
---|---|---|
382–383 | is there a reason why we can't omit the region in this op? just have test.buffer_based in(%arg0: memref<5xf32>) out(%y: memref<5xf32>); and %1 = test.tensor_based in(%arg0 : tensor<5xf32>) -> tensor<5xf32> ` | |
mlir/test/lib/Transforms/TestBufferPlacement.cpp | ||
69 ↗ | (On Diff #301585) | nit: both the original and the rewritten op are "test" ops, maybe rename testOp -> bufferOp |
75 ↗ | (On Diff #301585) | would Block* oldBlock = op.getBody(); work? |
mlir/test/Transforms/promote-buffers-to-stack.mlir | ||
---|---|---|
382–383 | There are some tests with nested regions and allocs inside those regions, thus we need the region for those tests. But we could implement two OPs which inherit from a base Op, one with and one without a region. This would simplify the tests themselves, but complicate the TestOps. |
is there a reason why we can't omit the region in this op?
just have
and