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

Sun Feb 2 11:37:44 PST 2020

nicolasvasilache marked 42 inline comments as done.
nicolasvasilache added inline comments.

================
Comment at: mlir/docs/Dialects/Linalg.md:3

-To generate the documentation:
+Table of contents:
+1. [Introduction: Inception and Evolution](#introduction)
----------------
nicolasvasilache wrote:
> rriddle wrote:
> > It'd be really nice if we can use a TOC equivalent.
> Do you have a suggestion here? 
> If there is a canned solution for this I'd happily adopt it.
> I wouldn't want to condition progress on missing infrastructure and I am not looking for new pet projects at this time :) 
Trying with just TOC, we'll see how this goes.

================
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)
----------------
ftynse wrote:
> 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.
Will move the whole doc to a Rationale and then move things around a bit in a followup.

================
Comment at: mlir/docs/Dialects/Linalg.md:59
+the field, from which it seeks to learn key lessons. This documentation
+and introspection effort also comes in the context of the proposal for a
+working group for discussing the [Development of high-level Tensor Compute
----------------
rriddle wrote:
> This start of this paragraph seems like it's describing why you are writing this documentation, which seems weird for a doc about the dialect. 
Moving to a Rationale as suggested by others too.

================
Comment at: mlir/docs/Dialects/Linalg.md:118
+It is complemented by the
+[Vector dialect](https://mlir.llvm.org/docs/Dialects/Vector/),
+which define structured operations on vectors, following the same rationale and
----------------
rriddle wrote:
> Prefer relative paths on all of the things that can use it
I'll probably mess this up if I just go for it without a way to test.
Atm I am iterating with testing this with Markdown on github.
I prefer to leave the paths for a NFC followup. 

================
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,
----------------
ftynse wrote:
> Provide examples of this needs, to be concrete :)
These are Core Design Principles that will want to evolve into generic design principles that should also inform TCP (T Comp. Primitives) and other things. I don't want to oversubscribe this to Linalg (I may already do that too much in other places, I'll have to rework to make it into a design principles proposal).

================
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
----------------
ftynse wrote:
> I'm not sure it makes sense to put observations after principles. IMO, principles can be built on the observations (and prior art).
Historically I started from the principles and after some iterations trying to resolve apparent contradictions, these observations allowed me to define the IR the way it is.
As you and others have advocated, this doc will be split up into more pieces but I want the connections between pieces to be visible and clear at all times.

I'm not worrying about presentation ordering for now since things will move.

================
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
----------------
ftynse wrote:
> This paragraph sounds overlapping with "preservation of information" from the section above.
I reflowed the text into more appropriate places, thanks!

================
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.
----------------
ftynse wrote:
> Care to elaborate? I don't grasp this analogy.
Reshuffled things a bit around and explained a little more.

================
Comment at: mlir/docs/Dialects/Linalg.md:632
+and *schedule trees* in
+[ISL](https://en.wikipedia.org/wiki/Integer_set_library).
+
----------------
ftynse wrote:
> 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?
Yes you're right, dropping the reference to ISL here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73595