This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Introduce allowMemrefFunctionResults for the helper operation converters of buffer placement
ClosedPublic

Authored by dfki-ehna on Jun 4 2020, 3:16 AM.

Details

Summary

This parameter gives the developers the freedom to choose their desired function signature conversion for preparing their functions for buffer placement. It is introduced for BufferAssignmentFuncOpConverter, and also for BufferAssignmentReturnOpConverter, and BufferAssignmentCallOpConverter to adapt the return and call operations with the selected function signature conversion. If the parameter is set, buffer placement won't also deallocate the returned buffers.
Here are the examples if it is set and unset:

Input IR:

func test(%arg0: tensor<4x8xf32>) -> tensor<4x8xf32> {
  return %arg0 : tensor<4x8xf32>
}
func caller(%arg0: tensor<4x8xf32>) {
  %0 = call @test(%arg0) : (tensor<4x8xf32>) -> (tensor<4x8xf32>)
  return
}

Transformed IR (allowMemrefFunctionResults = true):

func test(%arg0: memref<4x8xf32>) -> memref<4x8xf32> {
  return %arg0 : memref<4x8xf32>
}
func caller(%arg0: memref<4x8xf32>) {
  %0 = call @test(%arg0) : (memref<4x8xf32>) -> (memref<4x8xf32>)
  return
}

Transformed IR (allowMemrefFunctionResults = false):

func test(%arg0: memref<4x8xf32>, %result: memref<4x8xf32) {
  copy(%arg0, %result)
  return
}
func caller(%arg0: memref<4x8xf32>) {
  %0 = alloc() : memref<4x8xf32>
  call @test(%arg0, %0) : (memref<4x8xf32>, memref<4x8xf32>) -> ()
  return
}

Diff Detail

Event Timeline

dfki-ehna created this revision.Jun 4 2020, 3:16 AM
dfki-ehna edited the summary of this revision. (Show Details)Jun 4 2020, 3:20 AM
dfki-ehna edited reviewers, added: herhut, pifon2a, mehdi_amini; removed: nicolasvasilache.

Before I start reviewing deeper. I there some special reason to use template argument to flip the flag instead of using pass options?
https://github.com/llvm/llvm-project/blob/0bfd70bdad7e4ac22d96503fa78a5dd55d4b430e/mlir/include/mlir/Dialect/SCF/Passes.td#L29

we would get a command line flag like in
https://github.com/llvm/llvm-project/blob/0bfd70bdad7e4ac22d96503fa78a5dd55d4b430e/mlir/test/Dialect/SCF/parallel-loop-tiling.mlir#L1

and the code might become clearer.

mlir/test/lib/Transforms/TestBufferPlacement.cpp
38

Maybe move

using BAFuncOpConverter =
     BufferAssignmentFuncOpConverter<allowMemrefFunctionResults>;
 using BAReturnOpConverter =
     BufferAssignmentReturnOpConverter<ReturnOp, ReturnOp, linalg::CopyOp,
                                       allowMemrefFunctionResults>;
 using BACallOpConverter =
     BufferAssignmentCallOpConverter<allowMemrefFunctionResults>;

closer to insert<> call. In that case, just remove BA part:

using FuncOpConverter =
     BufferAssignmentFuncOpConverter<allowMemrefFunctionResults>;
 using ReturnOpConverter =
     BufferAssignmentReturnOpConverter<ReturnOp, ReturnOp, linalg::CopyOp,
                                       allowMemrefFunctionResults>;
 using CallOpConverter =
     BufferAssignmentCallOpConverter<allowMemrefFunctionResults>;
165

just out of curiosity:
will replacing Super:: with this-> work?

pifon2a requested changes to this revision.Jun 4 2020, 7:11 AM
This revision now requires changes to proceed.Jun 4 2020, 7:11 AM
rriddle added inline comments.Jun 4 2020, 10:24 AM
mlir/include/mlir/Transforms/BufferPlacement.h
117

Return the error directly.

pifon2a accepted this revision.Jun 5 2020, 1:36 AM
pifon2a added inline comments.
mlir/include/mlir/Transforms/BufferPlacement.h
105

let's not have default values for allowMemrefFunctionResults.

161

It might make sense to hide this behaviour behind some populatePatterns func template, so that the users won't add this template with different allowMemrefFunctionResults values.

222

though->through

mlir/test/Transforms/buffer-placement-preparation-allowed-memref-results.mlir
90

nit: reduce indentation so that it fits?

mlir/test/lib/Transforms/TestBufferPlacement.cpp
188

"test-buffer-placement-preparation-with-allowed-memref-results"? to make it consistent with the pass above.

mlir/tools/mlir-opt/mlir-opt.cpp
44

sort?

This revision is now accepted and ready to land.Jun 5 2020, 1:36 AM
dfki-ehna updated this revision to Diff 268748.Jun 5 2020, 4:57 AM

Address the comments.

dfki-ehna updated this revision to Diff 268750.Jun 5 2020, 5:07 AM
dfki-ehna marked 8 inline comments as done.

Correct the position of registerTestPreparationPassWithAllowedMemrefResults.

dfki-ehna marked 4 inline comments as done.Jun 5 2020, 5:24 AM

Thanks for suggesting to use the Pass option. I made TestBufferPlacementPreparationPass a template struct due to the fact that registerTestBufferPlacementPreparationPass and registerTestPreparationPassWithAllowedMemrefResults are two passes for only test purposes which shouldn't be publicly seen.

mlir/include/mlir/Transforms/BufferPlacement.h
161

populateWithBufferAssignmentOpConversionPatternswas introduced. All three operation conversion patterns were moved to detail namespace in case if somebody needs to call them selectively.

mlir/test/lib/Transforms/TestBufferPlacement.cpp
38

Replaced with populateWithBufferAssignmentOpConversionPatterns

165

Replaced.

pifon2a accepted this revision.Jun 5 2020, 6:06 AM
pifon2a added inline comments.
mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp
111

/*allowMemrefFunctionResults=*/false

This revision was automatically updated to reflect the committed changes.