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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added a comment.
This revision is now accepted and ready to land.

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



================
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; });
----------------
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!


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:107
+  // clang-format off
+  patterns->insert<
+                  FunctionAndBlockSignatureConverter,
----------------
Nit, it should now be possible do format it as
```
 patterns->insert<FunctionAndBlockSignatureConverter,
                  GenericOpConverter,
                  ReturnOpConverter>(context, placer, converter);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78996