[PATCH] D78996: [MLIR][LINALG] Convert Linalg to Linalg

[Ehsan via Phabricator via llvm-commits]

dfki-ehna marked an inline comment as done.
dfki-ehna added inline comments.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:122
+    // Mark return operations illegal as long as they return values.
+    target.addDynamicallyLegalOp<mlir::ReturnOp>(
+        [](mlir::ReturnOp returnOp) { return returnOp.getNumOperands() == 0; });
----------------
ftynse wrote:
> mravishankar wrote:
> > dfki-ehna wrote:
> > > ftynse wrote:
> > > > 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
> > > >   }
> > > > }
> > > > ```
> > > 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.
> > > 
> > > 
> > 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.
> Please add a big scary TODO it the comment that clearly describes the issue and you are good to go!
Please look at our proposed solution for this issue ([[ https://reviews.llvm.org/D79329 | Update the FunctionAndBlockSignatureConverter and NonVoidToVoidReturnOpConverter of Buffer Assignment]]).


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:107
+  // clang-format off
+  patterns->insert<
+                  FunctionAndBlockSignatureConverter,
----------------
ftynse wrote:
> Nit, it should now be possible do format it as
> ```
>  patterns->insert<FunctionAndBlockSignatureConverter,
>                   GenericOpConverter,
>                   ReturnOpConverter>(context, placer, converter);
> ```
Clang-format would still fit GenericOpConverter in front of FunctionAndBlockSignatureConverter. So, I kept the clang-format off/on before and after.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78996/new/

https://reviews.llvm.org/D78996