[PATCH] D73595: [mlir][Linalg][doc] Add Design Document for the Linalg Dialect

Wed Jan 29 06:26:32 PST 2020

ftynse added a comment.

In D73595#1846632 <https://reviews.llvm.org/D73595#1846632>, @nicolasvasilache wrote:

> @ftynse it seems approppriate for you to review parts you haven't touched yet though.
>  I think your review would still be very valuable for sections 3 - 8 (Core Guiding Principles - end).
>
> Thanks!

Reviewed until 6 inclusive. FWIW, I think it's a waste of time for me to write comments and for you to try and implement them instead of me just fixing the text directly.

================
Comment at: mlir/docs/Dialects/Linalg.md:47
+
+In his latest MLIR Open Design meeting presentation, Chris laid out a compelling
+[vision](https://drive.google.com/corp/drive/folders/1C3SEjO4u9E0UB2IwztSxaovFTSATDxT6)
----------------
rriddle wrote:
> This seems like it's going to be out-of-date soon and feels weird to include for the documentation of a dialect.
I would consider putting everything up to and including section 5 into a separate "Rationale" doc that explains _why_ Linalg. And keep the sections 6-7 here to explain _how_ Linalg, referring the rationale doc the same way LangRef does.

================
Comment at: mlir/docs/Dialects/Linalg.md:406
+## Transformations and Simplicity First<a name="transformations_first"></a>
+The purpose of the Linalg IR and its operations is primarily to (1) develop a
+set of key transformations and (2) make them correct by construction (3) make
----------------
Make an actual list

================
Comment at: mlir/docs/Dialects/Linalg.md:407
+The purpose of the Linalg IR and its operations is primarily to (1) develop a
+set of key transformations and (2) make them correct by construction (3) make
+them very simple to implement, apply, verify and especially maintain.
----------------
"(1) A and B (2) (3) C, D and E" breaks the logical list structure here

================
Comment at: mlir/docs/Dialects/Linalg.md:407
+The purpose of the Linalg IR and its operations is primarily to (1) develop a
+set of key transformations and (2) make them correct by construction (3) make
+them very simple to implement, apply, verify and especially maintain.
----------------
ftynse wrote:
> "(1) A and B (2) (3) C, D and E" breaks the logical list structure here
"correct by construction by carefully curating the set of generic operation properties that drive applicability"

================
Comment at: mlir/docs/Dialects/Linalg.md:415
+involves considerations related to:
+- concrete current and future needs of the application domain,
+- concrete current and future hardware properties and ISAs,
----------------
Provide examples of this needs, to be concrete :)

================
Comment at: mlir/docs/Dialects/Linalg.md:441
+and cross a wide abstraction gap in a single step. This process often drops
+semantic information that will later needs to be reconstructed,
+when it is not irremediably lost.
----------------
grammo "will later needs to be reconstructed"

================
Comment at: mlir/docs/Dialects/Linalg.md:487
+properties to describe these semantics, directly in MLIR, is a promising way to:
+- Design transformations that are (1) correct by construction, (2) easy to
+write, (3) easy to verify and (4) easy to maintain.
----------------
I'd drop the inner list here

================
Comment at: mlir/docs/Dialects/Linalg.md:499
+## Suitability for Search and Machine Learning<a name="ml"></a>
+The concept of compiler heuristics bears similarities to hand-crafted
+human-engineered features: it is ripe for disruption by machine-learning
----------------
It isn't just a similarity, heuristics _are_ human-engineered features :)

================
Comment at: mlir/docs/Dialects/Linalg.md:503
+[composable](#declarative_transformations) and expose tuning parameters that
+can modify their behavior. Previous experience with Tensor Comprehensions showed
+that, fixing a few predefined strategies with parametric transformations and
----------------
We don't seem to have Tensor Comprehensions in the "lessons learned from". IMO, we should and part of this text can go there, together with the limitations.

================
Comment at: mlir/docs/Dialects/Linalg.md:515
+blackbox. For these reasons we prioritize the design of IR and transformations
+with search-friendly properties over building cost models.
+
----------------
We are also not saying we don't want cost models at all.  There are ways of having ML build you a cost model, or using a cost model to train ML.  Cite Hugh Leather and team.

================
Comment at: mlir/docs/Dialects/Linalg.md:518
+## Extensibility and Future-Proofness<a name="future"></a>
+MLIR defines IR for structured control flow and structured data types. In
+particular, the `MemRefType` represents dense non-contiguous memory regions.
----------------
MLIR merely enables one to do so, we chose to define it in the structured way for the reasons above.

================
Comment at: mlir/docs/Dialects/Linalg.md:527
+characterize statically. As a consequence we need to also design solutions that
+stand a chance of evolving into runtime and inspector-executor style of
+computations. While there is no concrete solution today to solve these problems
----------------
Explain what inspector-executor style means.

================
Comment at: mlir/docs/Dialects/Linalg.md:532
+
+# Key Observations<a name="keyobservation"></a>
+The following key observations have influenced the design of Linalg and helped
----------------
I'm not sure it makes sense to put observations after principles. IMO, principles can be built on the observations (and prior art).

================
Comment at: mlir/docs/Dialects/Linalg.md:543
+data structures: one cannot exist without the other. This has multiple
+implications on the [semantics of Linalg Ops](#linalg_ops).
+
----------------
I like the reference, but it would be better to briefly explain what the implications are. Something about choosing where to implement a transformation: on the algorithm or on the data?

================
Comment at: mlir/docs/Dialects/Linalg.md:545
+
+## Preserving Structure in the IR<a name="preserving_structure"></a>
+Perfectly nested loops form a particularly important class of structure that
----------------
This paragraph sounds overlapping with "preservation of information" from the section above.

================
Comment at: mlir/docs/Dialects/Linalg.md:565
+experimenting with different alternatives, it became clear that strict dialect
+closed-ness wasn't necessary and could be relaxed.
+
----------------
Maybe argue that closedness was necessary because one couldn't easily mix different types of IRs?  Other reasons?

================
Comment at: mlir/docs/Dialects/Linalg.md:567
+
+In practice, MLIR is designed as an infrastructure for ***progressive
+lowering***. Linalg fully embraces this notion and thinks of codegen in terms of
----------------
Progressive lowering is also one of the guiding principles above.  There's _some_ overlap between this section and the previous section. I would consider either merging the two, or explicitly deriving one from the other, point by point.

================
Comment at: mlir/docs/Dialects/Linalg.md:569
+lowering***. Linalg fully embraces this notion and thinks of codegen in terms of
+*reducing some potential function*. At this time, this potential is not yet
+well-defined but the analogy with physics is still relevant.
----------------
Care to elaborate? I don't grasp this analogy.

================
Comment at: mlir/docs/Dialects/Linalg.md:577
+transformation would dictate that the potential remains constant, Linalg 
+advocates for ***monotonicity*** under transformations.
+
----------------
Monotonicity of what? Do you mean monotonously non-increasing level of abstraction, i.e. after transformation you are either at the same level of abstraction, or at a mix of same and lower level of abstraction?

================
Comment at: mlir/docs/Dialects/Linalg.md:579
+
+Very concretely, despite the fact that GenericOp only allow perfectly nested
+semantics, once tiling and fusion kick in, imperfectly nested loops are gradually
----------------
You haven't introduced GenericOp yet.

================
Comment at: mlir/docs/Dialects/Linalg.md:593
+The textual form description of these transformations is left for future
+work. Still, it is useful to at list the key transformations that are
+performed on the Linalg IR and that have influenced its design:
----------------
typo "to at list"

================
Comment at: mlir/docs/Dialects/Linalg.md:618
+) abstraction on tensors and buffers. This is architected as two generic operations
+`linalg.generic` (resp. `linalg.indexed_generic`) that can expressing custom
+operations with *index-free semantics* (resp. *indexing semantics*).
----------------
typo: can expressing

================
Comment at: mlir/docs/Dialects/Linalg.md:632
+and *schedule trees* in
+[ISL](https://en.wikipedia.org/wiki/Integer_set_library).
+
----------------
Isn't it exactly the inverse in schedule trees? The op (leaf stmt) has almost no information and  you need to trace to the tree root to collect it?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73595