Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linalg generic-print named ops with hidden regions to match upstream linalg #2959

Open
2 of 7 tasks
n-io opened this issue Jul 30, 2024 · 1 comment
Open
2 of 7 tasks
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@n-io
Copy link
Collaborator

n-io commented Jul 30, 2024

PR #2957 describes a discrepancy between the linalg dialect re-implemented in xdsl and the upstream linalg dialect.

Specifically, linalg named ops are printed correctly in the default pretty printing mode, but the generic printing mode should also print a hidden region.

The above PR #2957 makes the xdsl-opt generic print for AddOp, SubOp, and MulOp, which previously was:

xdsl-opt --print-op-generic tests/filecheck/mlir-conversion/with-mlir/dialects/linalg/ops.mlir

[...]
%sum = "linalg.add"(%2, %2, %3) <{"operandSegmentSizes" = array<i32: 2, 1>}> : (tensor<2x3xf32>, tensor<2x3xf32>, tensor<2x3xf32>) -> tensor<2x3xf32>

... match the mlir-opt generic print:

mlir-opt --mlir-print-op-generic tests/filecheck/mlir-conversion/with-mlir/dialects/linalg/ops.mlir

[...]
%2 = "linalg.add"(%1#0, %1#0, %1#1) <{operandSegmentSizes = array<i32: 2, 1>}> ({
^bb0(%arg0: f32, %arg1: f32, %arg2: f32):
  %21 = "arith.addf"(%arg0, %arg1) <{fastmath = #arith.fastmath<none>}> : (f32, f32) -> f32
  "linalg.yield"(%21) : (f32) -> ()
}) : (tensor<2x3xf32>, tensor<2x3xf32>, tensor<2x3xf32>) -> tensor<2x3xf32>

Help is wanted in checking if the generic print differs for the following ops between xdsl-opt and mlir-opt, and to fix this in the code:

  • FillOp
  • TransposeOp
  • MatmulOp
  • PoolingNchwMaxOp
  • Conv2DNchwFchwOp
  • BroadcastOp
  • Is there a non-manual way to verify MLIR compatibility (e.g. in tests/filecheck/mlir-conversion/with-mlir/dialects/linalg/ops.mlir) ?

The NamedOpBase class provides standard printing/parsing methods for the common ins/outs+variadic syntax employed by linalg named ops, and may be re-used for any of these ops.

@n-io n-io added bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed labels Jul 30, 2024
@jorendumoulin
Copy link
Collaborator

#2971 addresses matmul and fill op!

jorendumoulin added a commit that referenced this issue Aug 1, 2024
…2971)

I happened to need the fill operator and decided to do the matmul as
well. This continues the work of #2959
I also made sure that the code works on more than just floating points
values.
I moved some duplicate code to determine the type arguments of the
hidden region into a common method of the `NamedOp` base class.
I also used the implicit builder to build the regions as I think this is
much more readable.

Doing the matmul was a bit more tricky:

- for some reason this op prints the attributes in front of the `ins`
and `outs`
- generic from includes the `linalg.memoized_indexing_maps` attribute,
which are the indexing maps that would be generated when converting to a
linalg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants