Page MenuHomePhabricator

[MLIR][LINALG] Convert Linalg to Linalg
ClosedPublic

Authored by dfki-ehna on Apr 28 2020, 5:55 AM.

Details

Summary

This is a basic pass to convert Linalg.GenericOp which works on tensors to use buffers instead.

Diff Detail

Event Timeline

dfki-ehna created this revision.Apr 28 2020, 5:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 5:55 AM
dfki-ehna added a reviewer: pifon2a.
dfki-ehna added a project: Restricted Project.
ftynse requested changes to this revision.Apr 28 2020, 6:28 AM
ftynse added a subscriber: ftynse.

lib/Conversion is intended to for conversions from dialect A to dialect B to ensure it can depend on both without creating unnecessary dependencies A->B or B->A. Since this transformation stays within Linalg, it should go to lib/Dialect/Linalg/Transforms. Beyond being cleaner library-wise, it can have a more meaningful name than "linalg to linalg", e.g. "TensorsToBuffers" or "BufferAssignment".

This revision now requires changes to proceed.Apr 28 2020, 6:28 AM

just a suggestion: call the pass ConvertLinalgOnTensorsToBuffers?

pifon2a added inline comments.Apr 28 2020, 8:36 AM
mlir/lib/Conversion/LinalgToLinalg/LinalgToLinalg.cpp
74 ↗(On Diff #260605)

shouldn't it be rewriter.replaceOp(op, <what to replace results with>)? Please, add a test with two chained Linalg ops.

103 ↗(On Diff #260605)

why is the definition of runOnOperation() method not inside TensorLinalgToBufferLinalgPass? I don't think it improved readability here. Is there some guide that does not allow doing that?

just a suggestion: call the pass ConvertLinalgOnTensorsToBuffers?

+1.

rriddle added inline comments.Apr 28 2020, 10:37 AM
mlir/lib/Conversion/LinalgToLinalg/LinalgToLinalg.cpp
135 ↗(On Diff #260605)

Just return applyFullConversion directly. failure converts to interrupt

dfki-ehna updated this revision to Diff 260890.Apr 29 2020, 5:09 AM

Address the comments.

dfki-ehna marked 5 inline comments as done.Apr 29 2020, 5:16 AM

lib/Conversion is intended to for conversions from dialect A to dialect B to ensure it can depend on both without creating unnecessary dependencies A->B or B->A. Since this transformation stays within Linalg, it should go to lib/Dialect/Linalg/Transforms. Beyond being cleaner library-wise, it can have a more meaningful name than "linalg to linalg", e.g. "TensorsToBuffers" or "BufferAssignment".

Thanks. The files were moved to the correct locations.

mlir/lib/Conversion/LinalgToLinalg/LinalgToLinalg.cpp
74 ↗(On Diff #260605)

Thanks and resolved. We were replacing the results' uses without rewriter using replaceAllUsesWith which was unsafe.

103 ↗(On Diff #260605)

Resolved. The reason for doing this was consistency with some other CPP files in the lib/conversion directory.

Here's a more detailed review. I am not familiar enough with the buffer assignment mechanism that was introduced, deferring that to @pifon2a

mlir/include/mlir/Dialect/Linalg/Passes.h
20

These forward declarations look unnecessary in this file

mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp
45

Please, call .reserve on vectors before using push_back in a loop

48

rewriter.notifyMatchFailure

If this is an invariant of the op itself, it should be checked in the verifier and can be asserted to be correct here.

71

Nit: we prefer auto in places where it improves readability, i.e. if the type is too long to spell out (e.g., iterators, zippers) or if it is obvious due to a cast. I think Region & would be just fine here.

78

.addArgument (and any inplace modification) is not allowed in a conversion pattern because it's not undoable. You'll have to create a new block with rewriter.createBlock and move/clone the operations from the existing block.

87

LLVM prefers declaring functions static to putting them in an anonymous namespace, unlike classes.

94–95

Could you have using <foo> = NonVoidToVoidReturnOpConverter<mlir::ReturnOp, mlir::ReturnOp, linalg::CopyOp> and put just <foo> here to get less crazy formatting? Don't know what's the name for <foo> though.

103

Given that the pass is declaratively specified, use ConvertLinalgOnTensorsToBuffersPassBase<ConvertLinalgOnTensorToBuffers> instead. (also feel free to choose a shorter name for the anonymous class)

114

Style nit: let's put this lambda outside the function call

123

This looks like it would make impossible to have simple helper functions that have no Linalg in them.

func @square(%arg0: f32) {
  %0 = mulf %arg0, %arg0 : f32
  return %0 : f32
}

func @foo(...) {
  linalg.generic (...buffers...) {
    %0 = call @square(...) : (f32) -> f32
    linalg.yield %0 : f32
  }
}
127

Similar to the above, but I might understand that we want this pass to fail if there remains any function on tensors (maybe it doesn't use linalg at all...)

mlir/test/Dialect/Linalg/tensors-to-buffers.mlir
16

Do we actually care about lines being strictly consecutive, or just them being ordered suffices? If the latter, drop the -NEXT.

ftynse added inline comments.Apr 29 2020, 9:04 AM
mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp
78

I filed a feature request to make this easier, PR45738. But this needs to be correct in the meantime.

ftynse requested changes to this revision.Apr 29 2020, 9:04 AM
This revision now requires changes to proceed.Apr 29 2020, 9:04 AM
dfki-ehna updated this revision to Diff 261235.Apr 30 2020, 8:10 AM
dfki-ehna marked 2 inline comments as done.

Address the comments.

dfki-ehna marked 13 inline comments as done.Apr 30 2020, 8:33 AM
dfki-ehna added inline comments.
mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp
123

Thanks for mentioning this. We addressed this issue with a quick fix to cover more test cases in this PR. However, the combination of FunctionAndBlockSignatureConverter and NonVoidToVoidReturnOpConverter of BufferAssignment would still fail in cases like the following:

func @function(%arg0: f32, %arg1: tensor<4xf32>) -> (f32, f32) {
  %0 = mulf %arg0, %arg0 : f32
  return %0, %0 : f32, f32
}

FunctionAndBlockSignatureConverter converts the function to:

func @function(%arg0: f32, %arg1: memref<4xf32>, f32, f32) {
  %0 = mulf %arg0, %arg0 : f32
  return %0, %0 : f32, f32
}

but the return operation remains unchanged and leaves the IR in a broken state.
These converters must be fixed more fundamentally and we are going to do so in the next PR.

Thanks. This is really useful!

mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp
123

Understand that this will be fixed later, but if a function is returning a scalar of scalarType, the modified function should have an argument that is memref<scalarType>. Otherwise I am not sure how to maintain the return semantics.

190

Nit: newline at end of file.

mlir/test/Dialect/Linalg/tensors-to-buffers.mlir
76

Nit: newline at end of file.

ftynse accepted this revision.Apr 30 2020, 12:19 PM

Thanks!
Don't forget to update the commit message that still says "convert linalg to linalg".

mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp
108

Nit, it should now be possible do format it as

patterns->insert<FunctionAndBlockSignatureConverter,
                 GenericOpConverter,
                 ReturnOpConverter>(context, placer, converter);
123

Please add a big scary TODO it the comment that clearly describes the issue and you are good to go!

This revision is now accepted and ready to land.Apr 30 2020, 12:19 PM
This revision was automatically updated to reflect the committed changes.
dfki-ehna marked 4 inline comments as done.
dfki-ehna marked an inline comment as done.May 5 2020, 7:03 AM
dfki-ehna added inline comments.
mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp
108

Clang-format would still fit GenericOpConverter in front of FunctionAndBlockSignatureConverter. So, I kept the clang-format off/on before and after.

123