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

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

103

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

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

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

103

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 ↗(On Diff #260890)

These forward declarations look unnecessary in this file

mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp
44 ↗(On Diff #260890)

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

47 ↗(On Diff #260890)

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.

70 ↗(On Diff #260890)

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.

77 ↗(On Diff #260890)

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

86 ↗(On Diff #260890)

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

93–94 ↗(On Diff #260890)

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.

102 ↗(On Diff #260890)

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

113 ↗(On Diff #260890)

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

122 ↗(On Diff #260890)

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
  }
}
126 ↗(On Diff #260890)

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
15 ↗(On Diff #260890)

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
77 ↗(On Diff #260890)

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
122 ↗(On Diff #260890)

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
166 ↗(On Diff #261235)

Nit: newline at end of file.

122 ↗(On Diff #260890)

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.

mlir/test/Dialect/Linalg/tensors-to-buffers.mlir
76 ↗(On Diff #261235)

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
107 ↗(On Diff #261235)

Nit, it should now be possible do format it as

patterns->insert<FunctionAndBlockSignatureConverter,
                 GenericOpConverter,
                 ReturnOpConverter>(context, placer, converter);
122 ↗(On Diff #260890)

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
107 ↗(On Diff #261235)

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

122 ↗(On Diff #260890)