Page MenuHomePhabricator

[MLIR][Linalg] Fixed obsolete examples in the MLIR Linalg Dialect doc
ClosedPublic

Authored by kumasento on Tue, Jul 21, 4:02 AM.

Details

Summary

This diff fixes some obsolete examples in the Linalg dialect documentation: https://mlir.llvm.org/docs/Dialects/Linalg/

These examples are used to explain the basic properties of the Linalg dialect, which are not automatically generated from TableGen and are using out-of-date MLIR/Linalg syntax.

This diff extends each example by adding essential attributes and changing its syntax to make it processible by mlir-opt. There is also a command attached to each example that says how the example can be processed.

Diff Detail

Event Timeline

kumasento created this revision.Tue, Jul 21, 4:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jul 21, 4:02 AM
ftynse accepted this revision.Tue, Jul 21, 4:13 AM
ftynse added a subscriber: ftynse.

Thanks! Please address the comments and this is good to go.

mlir/docs/Dialects/Linalg.md
74

Nit: we usually indent blocks the same as the parent operation, i.e.

linalg.generic... {
^bb0(...):
  "nested_op"() ...
}
96–111

Nit: while mlir-opt does produce an explicit module op, it is not mandatory in the input (an implicit top-level module is assumed) and superfluous to understanding this example. Let's not introduce it.

97

Let's not drop the layout. It's important to mention that the memref _may_ have one, without necessarily caring which one. You can use #layout1 and mention it's an attribute alias defined somewhere above.

This revision is now accepted and ready to land.Tue, Jul 21, 4:13 AM
kumasento updated this revision to Diff 279489.Tue, Jul 21, 5:04 AM

Finished fixing other examples in the properties section.

kumasento marked 2 inline comments as done.Tue, Jul 21, 5:10 AM
kumasento added inline comments.
mlir/docs/Dialects/Linalg.md
74

Thank you :) I've changed the styling of those mlir snippets.

96–111

Thank you @ftynse ! I've removed all module mentioned.

kumasento marked 3 inline comments as done.Tue, Jul 21, 5:23 AM
kumasento added inline comments.
mlir/docs/Dialects/Linalg.md
97

Hi @ftynse

Thank you for your comment. But I'm not sure whether it is still necessary to add the layout after I've already specified the indexing_maps in the #attrs. I kind of assume that layout is recommended to be generated from indexing_maps instead of writing by hand, based on some of the unittests in https://github.com/llvm/llvm-project/blob/master/mlir/test/Dialect/Linalg/standard.mlir, but I'm not very certain about this. If you would prefer explicitly specifying the layout, I will update these examples accordingly :)

kumasento edited the summary of this revision. (Show Details)Tue, Jul 21, 1:40 PM
ftynse added inline comments.Thu, Jul 23, 4:33 AM
mlir/docs/Dialects/Linalg.md
97

Thank you for your comment. But I'm not sure whether it is still necessary to add the layout after I've already specified the indexing_maps in the #attrs. I kind of assume that layout is recommended to be generated from indexing_maps instead of writing by hand, based on some of the unittests in https://github.com/llvm/llvm-project/blob/master/mlir/test/Dialect/Linalg/standard.mlir, but I'm not very certain about this. If you would prefer explicitly specifying the layout, I will update these examples accordingly :)

The memref layout is independent of indexings. The former describes the property of the data (most use cases fit into the strided layout scheme) while the latter is a property of the operation. Matrix multiplication works equally well on contiguous and non-contiguous memrefs with transpositions. The two properties contribute to the address calculation and have different expressivity trade-offs. For instance, indexings for matmul must be a permutation of inputs so they cannot express strided access, but the memref itself may have a strided layout. This is of utmost importance for transformations because strided layout do appear as the result of tiling and we must make clear Linalg supports memrefs with strided layouts.

kumasento updated this revision to Diff 280152.Thu, Jul 23, 8:57 AM

Added memory layouts to the first two examples.

kumasento marked an inline comment as done.Thu, Jul 23, 9:02 AM
kumasento added inline comments.
mlir/docs/Dialects/Linalg.md
97

Thank you @ftynse , I've added layout specification to the memref operands in the first two examples, please check them out when you're available.

I'm so sorry that I was very silly not being able to distinguish between the concept of layout and indexing. Now I fully understand that and I definitely agree with you that these examples should elaborate the possibility to specify memref layout for linalg.generic operation.

Please let me know if there is anything else I should improve, thanks!

kumasento retitled this revision from Fixed obsolete examples in the MLIR Linalg Dialect doc to [MLIR][Linalg] Fixed obsolete examples in the MLIR Linalg Dialect doc.Thu, Jul 23, 10:45 AM

Hi Alex @ftynse , if this diff looks ok would you mind helping me committing it? Thanks!