[PATCH] D72555: [mlir][Linalg] Update the semantics, verifier and test for Linalg with tensors.

[Stephan Herhut via Phabricator via llvm-commits]

herhut added a comment.

I browsed over the code and left some minor comments. The idea and code in verifiers LGTM. I find it much harder to read now with all those similar sounding accessors but that is inevitable I assume if you want to support mixed forms.

It seems that all transformation and code generation still assumes all buffer representations. That is fine (and also makes sense) but I'd prefer to have something emit an error or assert if this is not the case. One way less to shoot myself in the foot.



================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:694-697
       : tensor<?x?xf32>,
         memref<?x?xf32, stride_specification>,
         tensor<?x?xf32>
         -> (tensor<?x?xf32>)
----------------
So this example has a single output assuming that there is an order constraint on the arguments (inputs first, then outputs). Could the describe this here?


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:187
+  unsigned getNumInputsAndOutputBuffers() {
+    assert(this->getOperation()->getNumResults() <= nInputs() + nOutputs());
+    return nInputs() + nOutputs() - this->getOperation()->getNumResults();
----------------
The number of results alone needs to the smaller than nOutputs(). Maybe assert that?


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:184
+             << (idx + 1) << " of the same type as elemental type "
+             << shapedType.getElementType() << " of input " << (idx + 1);
+  }
----------------
This iterators over all operands, including output buffers.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:90
 public:
   static void emitScalarImplementation(ArrayRef<Value> allIvs, CopyOp copyOp) {
     auto nPar = copyOp.getNumParallelLoops();
----------------
This code assumes an all buffer representation? Maybe add an assert?


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:216
 template <typename IndexedValueType>
 class LinalgScopedEmitter<IndexedValueType, GenericOp> {
 public:
----------------
This code assumes an all buffers representation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72555