Page MenuHomePhabricator

[MLIR] Added test operations to replace linalg and scf in buffer placement tests.
ClosedPublic

Authored by dfki-albo on Oct 23 2020, 6:30 AM.

Details

Summary

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.

Diff Detail

Event Timeline

dfki-albo created this revision.Oct 23 2020, 6:30 AM
dfki-albo requested review of this revision.Oct 23 2020, 6:30 AM

Could you expand the description to add the rationale? It is mostly focussed on what but I don't know why

dfki-albo retitled this revision from [MLIR] Added test operations to replace scf and linalg. to [MLIR] Added test operations to replace linalg and scf in buffer placement tests..Oct 23 2020, 7:06 AM
dfki-albo edited the summary of this revision. (Show Details)
dfki-albo edited the summary of this revision. (Show Details)Oct 23 2020, 7:22 AM
dfki-albo edited the summary of this revision. (Show Details)
pifon2a requested changes to this revision.Mon, Oct 26, 4:46 AM
pifon2a added inline comments.
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.
Wouldn't

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

This revision now requires changes to proceed.Mon, Oct 26, 4:46 AM

Could you expand the description to add the rationale? It is mostly focussed on what but I don't know why

@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.

dfki-albo updated this revision to Diff 301585.Thu, Oct 29, 5:18 AM
dfki-albo marked 2 inline comments as done.

Addressed reviewer comments and reverted scf testdialect operations.

dfki-albo added inline comments.Thu, Oct 29, 5:20 AM
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?

pifon2a requested changes to this revision.Thu, Oct 29, 7:29 AM
This revision now requires changes to proceed.Thu, Oct 29, 7:29 AM
dfki-albo marked 2 inline comments as done.Fri, Oct 30, 1:50 AM
dfki-albo added inline comments.
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.

dfki-albo updated this revision to Diff 301893.Fri, Oct 30, 7:48 AM

Addressed reviewer comments.

dfki-albo marked an inline comment as done.Fri, Oct 30, 7:50 AM
dfki-albo updated this revision to Diff 302221.Mon, Nov 2, 12:53 AM

Fixed some outdated comments.

pifon2a accepted this revision.Mon, Nov 2, 1:02 AM

Much nicer! Thank you!

This revision is now accepted and ready to land.Mon, Nov 2, 1:02 AM