[mlir][Linalg] Wrong tile size for convolutions fixed

[limo1996]

Summary

Sizes of tiles (subviews) are bigger by 1 than they should. Let's consider
1D convolution without batches or channels. Furthermore let m iterate over
the output and n over the kernel then input is accessed with m + n. In tiling
subview sizes for convolutions are computed by applying requested tile size
together with kernel size to the above mentioned expression thus let's say
for tile size of 2 the subview size is 2 + size(n), which is bigger by one
than it should since we move kernel only once. The problem behind it is that
range is not turned into closed interval before the composition. This commit
fixes the problem by turning ranges first into closed intervals by substracting
1 and after the composition back to half open by adding 1.

PHAB_REVIEW=D86638

[mravishankar]

@limo1996 This is really nice find! I have been struggling with figuring out why convolutions are giving me errors in some cases, and I suspected something like, but wasnt able to nail this down.

There might be a better fix though. I'll take a look at this as well. For now I am going to just "Request changes" but I dont really have any changes to request. Just a placeholder for me to take a look again.

[limo1996]

In D86638#2239561, @mravishankar wrote:

@limo1996 This is really nice find! I have been struggling with figuring out why convolutions are giving me errors in some cases, and I suspected something like, but wasnt able to nail this down.

There might be a better fix though. I'll take a look at this as well. For now I am going to just "Request changes" but I dont really have any changes to request. Just a placeholder for me to take a look again.

Thank you @mravishankar! I know for sure there is a better fix but my goal here was to initiate a discussion.

You can try new Convolutions for different layouts and ranks that were recently added. I also saw that traditional linalg.conv is quite buggy but the plan is that it will be replaced so I did not invest time into fixing it.

[mravishankar]

Could we start a discourse post on this. Its more easy to document issues and discuss solutions there rather than on the patch.

Also, I am curious what other bugs you found in the convolution operation. Please add those to the discourse post if possible.

[limo1996]

In D86638#2242434, @mravishankar wrote:

Could we start a discourse post on this. Its more easy to document issues and discuss solutions there rather than on the patch.

Also, I am curious what other bugs you found in the convolution operation. Please add those to the discourse post if possible.

Here is the discourse post: https://llvm.discourse.group/t/wrong-tile-size-for-convolution-ops/1699

[limo1996]

Implementation changed as discussed here:
https://llvm.discourse.group/t/wrong-tile-size-for-convolution-ops/1699

[nicolasvasilache]

mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp

248

Please use mlir::edsc::op::operator+ and write it as size + std_constant_index(1) to get the proper affine_apply operation generated.
Without that things won't canonicalize nicely.
See e.g. mlir/test/EDSC/builder-api-test.cpp line 82 for some C++ and the generated IR.

289

same thing here re operator+

mlir/test/Dialect/Linalg/tile_simple_conv.mlir

38

these shouls not appear as subi / addi but instead use affine_apply with the properly canonicalized/simplified expressions.

[limo1996]

Comments of nicolas resolved

[nicolasvasilache]

Great, thank you @limo1996 !

[mravishankar]

Thanks!

mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp

static SmallVector<Value, 4> makeTiledViews(OpBuilder &b, Location loc,
 
   auto viewSizes = applyMapToValues(b, loc, map, allViewSizes);
   // Construct (potentially temporary) mins and maxes on which to apply maps
   // that define tile subviews.
   SmallVector<Value, 8> lbs, subViewSizes;
   for (unsigned idx = 0, idxIvs = 0, e = tileSizes.size(); idx < e; ++idx) {
     bool isTiled = !isZero(tileSizes[idx]);
     lbs.push_back(isTiled ? ivs[idxIvs++] : (Value)std_constant_index(0));
-    subViewSizes.push_back(isTiled ? tileSizes[idx] : viewSizes[idx]);
+    // Before composing, we need to make range a closed interval.
+    Value size = isTiled ? tileSizes[idx] : viewSizes[idx];
+    subViewSizes.push_back(size - std_constant_index(1));
   }
 
   auto *op = linalgOp.getOperation();
 
   SmallVector<Value, 4> res;
   res.reserve(op->getNumOperands());
   auto viewIteratorBegin = linalgOp.getInputsAndOutputBuffers().begin();
   for (unsigned viewIndex = 0; viewIndex < linalgOp.getNumInputsAndOutputs();
for (unsigned r = 0; r < rank; ++r) {
         continue;
       }
 
       // Tiling creates a new slice at the proper index, the slice step is 1
       // (i.e. the slice view does not subsample, stepping occurs in the loop).
       auto m = map.getSubMap({r});
       auto offset = applyMapToValues(b, loc, m, lbs).front();
       offsets.push_back(offset);
-      auto size = applyMapToValues(b, loc, m, subViewSizes).front();
+      auto closedIntSize = applyMapToValues(b, loc, m, subViewSizes).front();
+      // Resulting size needs to be made half open interval again.
+      auto size = closedIntSize + std_constant_index(1);
 
       // The size of the subview should be trimmed to avoid out-of-bounds
       // accesses, unless we statically know the subview size divides the view
       // size evenly.
       int64_t viewSize = viewType.getDimSize(r);
       auto sizeCst = size.getDefiningOp<ConstantIndexOp>();
       if (ShapedType::isDynamic(viewSize) || !sizeCst ||
           (viewSize % sizeCst.getValue()) != 0) {

mlir/test/Dialect/Linalg/tile_conv.mlir

 // RUN: mlir-opt %s -linalg-tile="linalg-tile-sizes=2,3,0,0,4" | FileCheck %s -check-prefix=TILE-23004
 
 // TILE-23004-DAG: #[[$D0x30pS0x10:.*]] = affine_map<(d0) -> (d0 * 30)>
-// TILE-23004-DAG: #[[$S0x10p90D0x30pS1:.*]] = affine_map<(d0)[s0, s1] -> (s0 * 10 + 90, d0 * -30 + s1)>
+// TILE-23004-DAG: #[[$S0x10p90D0x30pS1:.*]] = affine_map<(d0)[s0, s1] -> (s0 * 10 + 51, d0 * -30 + s1)>
 // TILE-23004-DAG: #[[$strided4D:.*]] = affine_map<(d0, d1, d2, d3)[s0, s1, s2, s3] -> (d0 * s1 + s0 + d1 * s2 + d2 * s3 + d3)>
 // TILE-23004-DAG: #[[$bound_map_4:.*]] = affine_map<(d0)[s0] -> (4, -d0 + s0)>
 
 func @conv(%arg0: memref<?x?x?x?xf32, offset: ?, strides: [?, ?, ?, 1]>, %arg1: memref<?x?x?x?xf32, offset: ?, strides: [?, ?, ?, 1]>, %arg2: memref<?x?x?x?xf32, offset: ?, strides: [?, ?, ?, 1]>) {
   linalg.conv(%arg0, %arg1, %arg2) {dilations = [10, 20], strides = [30, 40]} : memref<?x?x?x?xf32, offset: ?, strides: [?, ?, ?, 1]>, memref<?x?x?x?xf32, offset: ?, strides: [?, ?, ?, 1]>, memref<?x?x?x?xf32, offset: ?, strides: [?, ?, ?, 1]>
   return
 }
 //       TILE-23004: func @conv(

mlir/test/Dialect/Linalg/tile_simple_conv.mlir

 // RUN: mlir-opt %s -linalg-tile="linalg-tile-sizes=2,3,4" | FileCheck %s
 
 //  CHECK-DAG: #[[MAP0:.*]] = affine_map<(d0)[s0] -> (2, -d0 + s0)>
-//  CHECK-DAG: #[[MAP1:.*]] = affine_map<(d0)[s0, s1] -> (s0 + 3, -d0 + s1)>
-//  CHECK-DAG: #[[MAP2:.*]] = affine_map<(d0)[s0, s1] -> (s0 + 4, -d0 + s1)>
+//  CHECK-DAG: #[[MAP1:.*]] = affine_map<(d0)[s0, s1] -> (s0 + 2, -d0 + s1)>
+//  CHECK-DAG: #[[MAP2:.*]] = affine_map<(d0)[s0, s1] -> (s0 + 3, -d0 + s1)>
 //  CHECK-DAG: #[[MAP4:.*]] = affine_map<(d0)[s0] -> (3, -d0 + s0)>
 //  CHECK-DAG: #[[MAP5:.*]] = affine_map<(d0)[s0] -> (4, -d0 + s0)>
 
 func @conv(%arg0 : memref<?x?x?x?xf32>, %arg1 : memref<?x?x?x?xf32>, %arg2 : memref<?x?x?x?xf32>) {
   linalg.conv(%arg0, %arg1, %arg2) : memref<?x?x?x?xf32>, memref<?x?x?x?xf32>, memref<?x?x?x?xf32>
   return
 }
 

 //       CHECK:       scf.for %[[ARG5:.*]] = %[[C0]] to %[[T4]] step %[[C4]]
 //       CHECK:         %[[T5:.*]] = dim %[[ARG1]], %[[C0]]
 //       CHECK:         %[[T6:.*]] = affine.min #[[MAP0]](%[[ARG3]])[%[[T5]]]
 //       CHECK:         %[[T7:.*]] = dim %[[ARG1]], %[[C1]]
 //       CHECK:         %[[T8:.*]] = affine.min #[[MAP1]](%[[ARG4]])[%[[T0]], %[[T7]]]
 //       CHECK:         %[[T9:.*]] = dim %[[ARG1]], %[[C2]]
 //       CHECK:         %[[T10:.*]] = affine.min #[[MAP2]](%[[ARG5]])[%[[T1]], %[[T9]]]
 //       CHECK:         %[[T11:.*]] = dim %[[ARG1]], %[[C3]]
 //       CHECK:         %[[SV1:.*]] = subview %[[ARG1]][%[[ARG3]], %[[ARG4]], %[[ARG5]], 0]
 //  CHECK-SAME:                                        [%[[T6]], %[[T8]], %[[T10]], %[[T11]]]
 //       CHECK:         %[[T13:.*]] = dim %[[ARG2]], %[[C0]]
 //       CHECK:         %[[T14:.*]] = affine.min #[[MAP0]](%[[ARG3]])[%[[T13]]]
 //       CHECK:         %[[T15:.*]] = dim %[[ARG2]], %[[C1]]
 //       CHECK:         %[[T16:.*]] = affine.min #[[MAP4]](%[[ARG4]])[%[[T15]]]
 //       CHECK:         %[[T17:.*]] = dim %[[ARG2]], %[[C2]]
 //       CHECK:         %[[T18:.*]] = affine.min #[[MAP5]](%[[ARG5]])[%[[T17]]]
 //       CHECK:         %[[T19:.*]] = dim %[[ARG2]], %[[C3]]
 //       CHECK:         %[[SV2:.*]] = subview %[[ARG2]][%[[ARG3]], %[[ARG4]], %[[ARG5]], 0]
 //  CHECK-SAME:                                        [%[[T14]], %[[T16]], %[[T18]], %[[T19]]]
-//       CHECK:         linalg.conv(%[[ARG0]], %[[SV1]], %[[SV2]])
+//       CHECK:         linalg.conv(%[[ARG0]], %[[SV1]], %[[SV2]])