[PATCH] D77067: [mlir][Linalg] Create a tool to generate named Linalg ops from a Tensor Comprehensions-like specification.

Thu Apr 9 12:15:37 PDT 2020

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

================
Comment at: mlir/test/mlir-linalg-ods-gen/test-linalg-ods-gen.tc:5
+
+// RUN: mlir-linalg-ods-gen %s -gen-ods-decl=1 -o %t \
+// RUN: && cat %S/../../include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td %t \
----------------
ftynse wrote:
> silvas wrote:
> > silvas wrote:
> > > nicolasvasilache wrote:
> > > > ftynse wrote:
> > > > > nicolasvasilache wrote:
> > > > > > silvas wrote:
> > > > > > > layering-wise would prefer to not test this here. If needed, we can add a separate test elsewhere that does this .td -> .inc file check. Strictly speaking what ends up in the .inc file is not really the concern of this component, only the contents of the .td file.
> > > > > > > 
> > > > > > This was suggested by @ftynse to show the ODS is valid and how it connects to tblgen by mirroring this test: https://github.com/llvm/llvm-project/blob/master/mlir/test/mlir-tblgen/llvm-intrinsics.td#L11.
> > > > > > 
> > > > > > I am fine either way, would just like consensus on this before reverting back to the previous state.
> > > > > > Please reopen if you feel strongly about this.
> > > > > > @ftynse any strong opinion?
> > > > > I'm not sure I understand what is the concern here? The `ODS` check verifies the content of the produced .td file, _not_ the result of feeding that .td file to `mlir-tblgen -gen-op-defs`, which is indeed a separate concern. The `IMPL` check verifies the implementations of methods that are declared in the `.td` file and there is simply no other place where we can verify them.
> > > > > 
> > > > > The staging here is:
> > > > > 1a. mlir-linalg-ods-gen -gen-ods-decl %this_file% > ods.td
> > > > > 1b. mlir-linalg-ods-gen -gen-impl %this_file% > impl.cc
> > > > > 2a. mlir-tblgen -gen-op-decl ods.td > ods.h
> > > > > 2b. mlir-tblgen -gen-op-decl ods.td > ods.cc
> > > > > 3. include impl.cc and ods.cc into the implementation file; and ods.h into the header file.
> > > > > 
> > > > > @nicolasvasilache the test you referenced also has `RUN` lines making sure `mlir-tblgen` can consume what the first stage produces. Consider adding them here as well. This could help detect cases of ODS syntax change (the simple syntactic test passes, but not the piping check). That's why there is only a trivial check to make sure FileCheck eats something.
> > > > ```
> > > > I'm not sure I understand what is the concern here? 
> > > > ...
> > > > Consider adding them here as well.
> > > > ```
> > > > 
> > > > That's precisely what the concern was IIUC, piping through mlir-tblgen (see previous snapshot that I updated improperly https://reviews.llvm.org/D77067?id=254251).
> > > > 
> > > > Restored that part of the test.
> > > > 
> > > > 
> > > Ah, okay. Sorry for the confusion! When I saw C++ code I was assuming it was emitted by mlir-tblgen gen-op-def. But I see now that there is a mlir-linalg-tblgen -gen-impl that emits C++ as well. Sorry for the noise!!!
> > Actually, when rereading I see that we do indeed invoke `mlir-tblgen -gen-op-decls`. I specifically object to the `TBLGEN` check prefixes here. I consider it a bug to do that (although i see the precendent in llvm-intrinsics.td, but I would have raised the same objection there), since it violates the layering: somebody updating mlir-tblgen shouldn't be able to break this test.
> > 
> > Consider the implications of what is being checked now in this test...
> > 
> > ```
> > // TBLGEN-LABEL: linalg::batchmatmulOp declarations
> > ^ could be broken by a change in a *comment* in the generated file :x
> > //       TBLGEN: class batchmatmulOpOperandAdaptor {
> > ^ could be broken by adding a common base class to the operand adaptor classes, or a change in naming convention for the adaptor classes
> > //       TBLGEN: class batchmatmulOp : public Op<
> > ^ could be changed by a change in base classes or naming convention.
> > ```
> > 
> > Note that none of those changes I've indicated would actually break any actual use of this code. So this test is just artificially constraining the implementation of mlir-tblgen for no real value. And even if you strip it down, all you would really be testing is `def batchmatmulOp` results in a `class batchmatmulOp` in the output, which is already tested in many places, such as, say, https://github.com/llvm/llvm-project/blob/master/mlir/test/mlir-tblgen/op-decl.td
> > 
> > We need to be courteous to the maintainers of other components and give them the flexibility to adjust the implementations of their components.
> I agree that this specific test is over-constraining for mlir-tblgen implementation. What I intended to test in intrinsicgen, and what I would like to see replicated here, is that the tablegen input produced by intrinsicgen, or my mlir-linalg-ods-gen, can be consumed by mlir-tblgen at all. Basically, we don't need to check for any output if we can find a way to check that mlir-tblgen exited with code 0 on the produced file. FileChecking the class name is just a workaround.
> 
> If we don't do this check, we risk ending up in a situation where all of the existing tests pass (mlir-tblgen still generates expected C++ from ODS, and mlir-linalg-ods-gen still generates the strings expected by its test, just those strings are no longer valid ODS), but the pipeline fails. And given mlir-tblgen's tendency to assert or crash on improperly structured yet valid TableGen, it would be annoying to debug.
Updated the test to get the minimal checkable thing: the class name.

================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:859
+
+  static bool classof(const Expression *e) {
+    return e->kind == Kind::TensorUse;
----------------
ftynse wrote:
> nicolasvasilache wrote:
> > ftynse wrote:
> > > If you use LLVM-style type system, you would normally want to avoid virtual functions...
> > why ? https://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html#basic-setup shows it's perfectly fine to use abstract base classes and LLVM RTTI.
> Because you pay the runtime overhead price for two abstractions serving essentially the same goal. Why?
Marking as done, this is part of the more global reply on downcasting, uptr etc.

================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:910
+        return false;
+    for (unsigned i = 0, e = reductionDimensions.size(); i < e; ++i)
+      if (reductionDimensions[i] != other.reductionDimensions[i])
----------------
ftynse wrote:
> nicolasvasilache wrote:
> > ftynse wrote:
> > > Do you care about the order of reduction dimensions?
> > you should otherwise your computation is non-deterministic
> I suppose you mean the IR you produce does not have a deterministic order of dimensions, which makes it hard to check. TC semantics says that all dimensions are interchangable, so their textual order should not matter. If it does, we should discuss the semantics and avoid branding this input as TC-like.
Added a documentation section in `Linalg.md`.

================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:927
+  StringRef opId;
+  SmallVector<std::unique_ptr<Expression>, 4> expressions;
+  SetVector<unsigned> reductionDimensions;
----------------
ftynse wrote:
> nicolasvasilache wrote:
> > ftynse wrote:
> > > Do you actually need pointers? Can't we just store `Expression`s as is, eventually with appropriate move semantics to avoid extra copies?
> > It's this or uniqu'ing, underlying storage, placement new etc etc.
> > I went for the simple solution.
> > When we have strong data that we need to scale much more we can revisit. 
> I don't think vectors of unique_ptr are simpler than vectors of values. This is an extra abstraction with associated cognitive overhead. This is also one extra dynamic allocation per element, as opposed to occasional allocations in the vector, and no strong reason to maintain the pointer as unique or to auto-deallocate (other than you forced the allocation in the first place). There is no actual uniquing of expression, neither is there underlying storage or placement new, you seem to be mistaking this with how types/attributes are handled in MLIR.
unique_ptr + abstract base class: basically because I use downcasting.

`vector<Expression>` will slice unless derived classes have sizeof == 0 (i.e. there is an underlying pointer payload).

An option is to implement a similar arena + pImpl to what MLIR does for the "by-value" abstractions.
I consider this to be unnecessarily complex for my use case (parser that runs at compiler compile time):

`vector<unique_ptr<...>>` is a  standard and simple way to solve the slicing and ownership issue, its performance drawback are not relevant at this time IMO.

================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:1135
+  for (unsigned i = 0, e = tensors.size(); i < e; ++i)
+    if (tensors[i].name == id)
+      return tensors[i];
----------------
ftynse wrote:
> nicolasvasilache wrote:
> > ftynse wrote:
> > > Have you considered storing tensors in an llvm::StringMap indexed by name instead of doing linear lookups every time?
> > I need 2 extra maps and really don't anticipate a single named op to ever to a point where this would matter.
> > Of course if proven otherwise I'm happy to reconsider.
> Well, you currently have two extra vectors. I just don't see why prefer using a vector of pairs and implementing a search for _every one of them_ is better than using a dedicated container with accessor immediately available.
fair enough, done, thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77067