This is an archive of the discontinued LLVM Phabricator instance.

isReshapableDimBand: ignore strides of unit dims.
AbandonedPublic

Authored by Benoit on Nov 30 2021, 2:25 PM.

Details

Summary

See the included lit test: without this isReshapableDimBand change, it
fails to compile as it expects a different, dynamic-shape result memref
type:

mlir/test/Dialect/MemRef/ops.mlir:200:8: error: 'memref.collapse_shape' op expected collapsed type to be 'memref<?xf32, affine_map<(d0)[s0, s1] -> (d0 * s1 + s0)>>', but got 'memref<64xf32, affine_map<(d0)[s0] -> (d0 + s0)>>'
  %1 = memref.collapse_shape %arg [[0, 1, 2, 3]] : memref<1x1x8x8xf32, affine_map<(d0, d1, d2, d3)[s0, s1, s2] -> (d0 * s0 + d1 * s1 + s2 + d2 * 8 + d3)>> into memref<64xf32, affine_map<(d0)[s0] -> (d0 + s0)>>
       ^

Another thing is changed by this diff: dim -> idx in the
isDynamicSize check. I wasn't able to observe a practical impact of this
change on any test case, but it feels more correct? sizes[idx+1] is what
is being multiplied below, and if really dim had been intended, this
would have been moved out of the for loop, where dim is a constant.

Diff Detail

Event Timeline

Benoit created this revision.Nov 30 2021, 2:25 PM
Benoit requested review of this revision.Nov 30 2021, 2:25 PM
Benoit edited the summary of this revision. (Show Details)
Benoit updated this revision to Diff 391022.Dec 1 2021, 7:34 AM

newline at end of file; minor wording update

mravishankar requested changes to this revision.Dec 2 2021, 2:03 PM
mravishankar added inline comments.
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
1226

Do we need the special check for sizes[idx] != 1. If a size is 1 then strides[idx+1] == strides[idx] which should be covered by the other part of th &&` check,

This revision now requires changes to proceed.Dec 2 2021, 2:03 PM
Benoit added a subscriber: herhut.Dec 2 2021, 8:00 PM

https://reviews.llvm.org/D114702 landed a few days ago and fixes the dim vs idx issue that I was reporting here! Thanks @herhut !

The part that remains here is the sizes[idx+1]!=1 check -- I will update the diff.

nicolasvasilache requested changes to this revision.Dec 3 2021, 7:46 AM
nicolasvasilache added inline comments.
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
1209

While you're touching this, could you please s/extent/numDims ?
Seems confusing now that I read it months later..

1226

Hmm, can't tell whether this is true or not without digging deeper.
This is a sign this needs to be reimplemented more simply.

Please use BuiltinTypes.h

AffineExpr makeCanonicalStridedLayoutExpr(ArrayRef<int64_t> sizes,
                                          MLIRContext *context);

Then extract the strides and offset and do a simple comparison of strides.drop_front(dim).take_front(numDims).

Please add missing helpers to allow these things to compose properly at the API level.

This revision now requires changes to proceed.Dec 3 2021, 7:46 AM
Benoit abandoned this revision.Dec 3 2021, 8:19 PM
Benoit added a subscriber: ThomasRaoux.

Thank you all for weighing in. After discussion with @nicolasvasilache and @ThomasRaoux today I switch to dropping the unit dims, which caused my need for the present diff, by inserting rank-reducing memref.subview in the pattern that is what generating the problematic memref.collapse_shape, see https://reviews.llvm.org/D114993 . So I can now close this diff, but thanks all for the conversation.