[PATCH] D136483: [mlir][MemRefToLLVM] Reuse existing lowering for collaspe/expand_shape

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

@springerm  thanks for your answer.

> But for dims of size 1 it doesn't really make sense to have a stride in the first place; so such a stride could be simplified, e.g., to just 1. Is that what's happening here?

With the new code what is happening is when we collapse dimensions, we take the stride of the innermost dimension as the stride of the whole collapsed dimension, since the underlying tensor is supposed to be contiguous.
e.g.,

  collapse_shape <5x?x?x?x?xi16, strided<[?, ?, ?, ?, ?]>>, [[0, 1], [2, 3, 4]] -> <?x?xi16>

>
=

  dim(0) == 5 x orig_shape.dim(1)
  dim(1) == orig_shape.dim(2) x  orig_shape.dim(3) x orig_shape.dim(4)
  ----
  stride(0) == orig_shape.stride(1)
  stride(1) == orig_shape.stride(4)

Now, this doesn't match what we were doing before this patch where in codegen we would explicitly skip the dimensions with size 1.
I.e., the old version would generate:

  dim(0) == 5 x orig_shape.dim(1)
  dim(1) == orig_shape.dim(2) x  orig_shape.dim(3) x orig_shape.dim(4)
  ----
  stride(0) == orig_shape.dim(1) != 1? orig_shape.stride(1) : orig_shape.stride(0)
  stride(1) == orig_shape.dim(4) != 1?
      orig_shape.stride(4) :
      (orig_shape.dim(3) != 1?
          orig_shape.stride(3):
          orig_shape.stride(2))

I understand that strides of dimensions of size 1 don't really make sense, but I found the old generated code to paper over something that is underspecified. Put differently, it actively harms codegen to have to ignore strides for dimension of size 1 and I was wondering if it is intended.

At my first reading of the spec, I was expecting that strides should be contiguous even for size 1 dimensions.

To take a concrete example from `@collapse_shape_dynamic_with_non_identity_layout`:

  %0 = memref.collapse_shape %arg0 [[0], [1, 2]]:
    memref<4x?x?xf32, strided<[?, 4, 1], offset: ?>> into
    memref<4x?xf32, strided<[?, ?], offset: ?>>

Here I was expecting that it is okay to infer that the inner most stride (`stride(1)`) is one, since we collapse dimensions with respectively strides 4 and 1. And if that's not true we are in the undefined behavior realm.
However with what you're saying, the stride could be either 1 or 4 depending whether `orig_shape.dim(2) == 1` or not.

Cheers,
-Quentin


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136483