This is an archive of the discontinued LLVM Phabricator instance.

Differential D114808

Enable ReassociatingReshapeOpConversion with "non-identity" layouts.
ClosedPublic

Authored by Benoit on Nov 30 2021, 9:49 AM.

Details

Reviewers
ftynse
mravishankar
nicolasvasilache
springerm
Commits
rG499703e9c08a: Enable ReassociatingReshapeOpConversion with "non-identity" layouts.
Summary

Enable ReassociatingReshapeOpConversion with "non-identity" layouts.

This removes an early-return in this function, which seems unnecessary and is
preventing some memref.collapse_shape from converting to LLVM (see included lit test).

It seems unnecessary because the return message says "only empty layout map is supported"
but there actually is code in this function to deal with non-empty layout maps. Maybe
it refers to an earlier state of implementation and is just out of date?

Though, there is another concern about this early return: the condition that it actually
checks, {src,dst}MemrefType.getLayout().isIdentity(), is not quite the same as what the
return message says, "only empty layout map is supported". Stepping through this
getLayout().isIdentity() code in GDB, I found that it evaluates to .getAffineMap().isIdentity()
which does (AffineMap.cpp:271):

if (getNumDims() != getNumResults())
  return false;

This seems that it would always return false for memrefs of rank greater than 1 ?

Diff Detail

Event Timeline

Benoit created this revision.Nov 30 2021, 9:49 AM
Herald added a reviewer: ftynse. · View Herald TranscriptNov 30 2021, 9:49 AM
Herald added subscribers: sdasgup3, wenzhicui, wrengr and 20 others. · View Herald Transcript
Benoit requested review of this revision.Nov 30 2021, 9:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2021, 9:49 AM
Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript
Benoit edited the summary of this revision. (Show Details)Nov 30 2021, 9:51 AM
Benoit added a reviewer: mravishankar.
mravishankar added reviewers: nicolasvasilache, springerm.Nov 30 2021, 9:52 AM
Harbormaster completed remote builds in B136723: Diff 390748.Nov 30 2021, 10:01 AM
Benoit updated this revision to Diff 390807.Nov 30 2021, 1:19 PM

Rename the test function - was not match what it actually did

Harbormaster completed remote builds in B136768: Diff 390807.Nov 30 2021, 1:48 PM
nicolasvasilache added a comment.Dec 3 2021, 7:07 AM

This is due to the weird trifecta between:

  1. memref without an affine_layout map being the convention to represent the canonical contiguous buffer of size equal to "the product of sizes"
  2. memref with an identity affine_map layout being by convention equivalent to no affine_layout
  3. memref with a strided layout that are currently represented by an affine_map.

This also intersects with the fact that a memref layout is provisioned to allow an arbitrarily complex composition of affine maps that I won't even get into.

I forget between the equivalence between 1. and 2. predates the strided memref or not (I think so but I am not 100% sure).

Things will get simpler once strided memref get back to using a different layout attribute offset:x, strides:[x,x,x] as originally intended.
However there are still many skeletons left because in the absence of a richer type/attribute system, it is unknown how the x in the strides relate to the ? in the sizes.
Properly encoding that relationship is key to being airtight in representing contiguous / non-contiguous layouts and reshapes with dynamic shapes.

Based on this info, please feel free to update docs so that it is less surprising to your future self and to others who will approach from the same angle as you did.

Thanks much!

mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
1148

See my longer comment but please only relax this in the fully static case.

in the absence of a richer type/attribute system, it is unknown how the `x` in the strides relate to the `?` in the sizes.
nicolasvasilache requested changes to this revision.Dec 3 2021, 7:48 AM
This revision now requires changes to proceed.Dec 3 2021, 7:48 AM
Benoit updated this revision to Diff 393903.Dec 13 2021, 8:06 AM

restrict to static shape case

Benoit added a comment.Dec 13 2021, 8:08 AM

Thanks for the review and explanation. I have updated the code to apply your inline comment i.e. restrict the change to the static shape case.
I haven't updated docs as you suggested because I'm out of my depth here, sorry.

Harbormaster completed remote builds in B138976: Diff 393903.Dec 13 2021, 8:31 AM
mravishankar added a comment.Dec 13 2021, 9:21 AM
In D114808#3169700, @nicolasvasilache wrote:

This is due to the weird trifecta between:

  1. memref without an affine_layout map being the convention to represent the canonical contiguous buffer of size equal to "the product of sizes"
  2. memref with an identity affine_map layout being by convention equivalent to no affine_layout
  3. memref with a strided layout that are currently represented by an affine_map.

This also intersects with the fact that a memref layout is provisioned to allow an arbitrarily complex composition of affine maps that I won't even get into.

I forget between the equivalence between 1. and 2. predates the strided memref or not (I think so but I am not 100% sure).

Things will get simpler once strided memref get back to using a different layout attribute offset:x, strides:[x,x,x] as originally intended.
However there are still many skeletons left because in the absence of a richer type/attribute system, it is unknown how the x in the strides relate to the ? in the sizes.
Properly encoding that relationship is key to being airtight in representing contiguous / non-contiguous layouts and reshapes with dynamic shapes.

Based on this info, please feel free to update docs so that it is less surprising to your future self and to others who will approach from the same angle as you did.

Thanks much!

Cant see how this is related to the change here, but I did run into this case today. The below operation currently fails roundtripping

%1 = memref.subview %0 [] [] [] : memref<?x?xf32> to memref<?x?xf32>

since the result subview expects a strided a layout when it shouldnt. I think it is because the subview creation method does not actually adhere to 1 and 2. I was planning to fix that.

nicolasvasilache accepted this revision.Dec 14 2021, 10:46 AM
This revision is now accepted and ready to land.Dec 14 2021, 10:46 AM
Benoit updated this revision to Diff 394363.Dec 14 2021, 1:03 PM

rebased

Harbormaster completed remote builds in B139291: Diff 394363.Dec 14 2021, 2:08 PM
Benoit added a comment.Dec 15 2021, 10:12 AM

I don't have permissions to submit this myself.

Benoit updated this revision to Diff 399702.Jan 13 2022, 9:32 AM

rebased and confirmed it is still needed to use vector transfer flattening.

Herald added a subscriber: awarzynski. · View Herald TranscriptJan 13 2022, 9:32 AM
This revision was landed with ongoing or failed builds.Jan 13 2022, 9:47 AM
Closed by commit rG499703e9c08a: Enable ReassociatingReshapeOpConversion with "non-identity" layouts. (authored by Benoit, committed by antiagainst). · Explain Why
This revision was automatically updated to reflect the committed changes.
antiagainst added a commit: rG499703e9c08a: Enable ReassociatingReshapeOpConversion with "non-identity" layouts..
Harbormaster completed remote builds in B143185: Diff 399702.Jan 13 2022, 9:48 AM

Revision Contents

PathSize
mlir/
lib/
Conversion/
MemRefToLLVM/
5 lines
test/
Conversion/
MemRefToLLVM/
10 lines

Diff 390748

mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp

Show First 20 Lines • Show All 1,139 Lines • ▼ Show 20 Linespublic:
using ConvertOpToLLVMPattern<ReshapeOp>::ConvertOpToLLVMPattern; using ConvertOpToLLVMPattern<ReshapeOp>::ConvertOpToLLVMPattern;
using ReshapeOpAdaptor = typename ReshapeOp::Adaptor; using ReshapeOpAdaptor = typename ReshapeOp::Adaptor;
LogicalResult LogicalResult
matchAndRewrite(ReshapeOp reshapeOp, typename ReshapeOp::Adaptor adaptor, matchAndRewrite(ReshapeOp reshapeOp, typename ReshapeOp::Adaptor adaptor,
ConversionPatternRewriter &rewriter) const override { ConversionPatternRewriter &rewriter) const override {
MemRefType dstType = reshapeOp.getResultType(); MemRefType dstType = reshapeOp.getResultType();
MemRefType srcType = reshapeOp.getSrcType(); MemRefType srcType = reshapeOp.getSrcType();
if (!srcType.getLayout().isIdentity() ||
nicolasvasilacheUnsubmitted
Not Done
ReplyInline Actions

See my longer comment but please only relax this in the fully static case.

in the absence of a richer type/attribute system, it is unknown how the `x` in the strides relate to the `?` in the sizes.
nicolasvasilache: See my longer comment but please only relax this in the fully static case. ``` in the absence…
!dstType.getLayout().isIdentity()) {
return rewriter.notifyMatchFailure(reshapeOp,
"only empty layout map is supported");
}
int64_t offset; int64_t offset;
SmallVector<int64_t, 4> strides; SmallVector<int64_t, 4> strides;
if (failed(getStridesAndOffset(dstType, strides, offset))) { if (failed(getStridesAndOffset(dstType, strides, offset))) {
return rewriter.notifyMatchFailure( return rewriter.notifyMatchFailure(
reshapeOp, "failed to get stride and offset exprs"); reshapeOp, "failed to get stride and offset exprs");
} }
▲ Show 20 Lines • Show All 435 Lines • Show Last 20 Lines

mlir/test/Conversion/MemRefToLLVM/memref-to-llvm.mlir

Show First 20 Lines • Show All 829 Lines • ▼ Show 20 Lines
// CHECK: llvm.insertvalue %{{.*}}, %{{.*}}[3, 2] : !llvm.struct<(ptr<f32>, ptr<f32>, i64, array<3 x i64>, array<3 x i64>)> // CHECK: llvm.insertvalue %{{.*}}, %{{.*}}[3, 2] : !llvm.struct<(ptr<f32>, ptr<f32>, i64, array<3 x i64>, array<3 x i64>)>
// CHECK: llvm.mlir.constant(1 : index) : i64 // CHECK: llvm.mlir.constant(1 : index) : i64
// CHECK: llvm.insertvalue %{{.*}}, %{{.*}}[4, 2] : !llvm.struct<(ptr<f32>, ptr<f32>, i64, array<3 x i64>, array<3 x i64>)> // CHECK: llvm.insertvalue %{{.*}}, %{{.*}}[4, 2] : !llvm.struct<(ptr<f32>, ptr<f32>, i64, array<3 x i64>, array<3 x i64>)>
// CHECK: llvm.mul %{{.*}}, %{{.*}} : i64 // CHECK: llvm.mul %{{.*}}, %{{.*}} : i64
// CHECK: llvm.insertvalue %{{.*}}, %{{.*}}[4, 1] : !llvm.struct<(ptr<f32>, ptr<f32>, i64, array<3 x i64>, array<3 x i64>)> // CHECK: llvm.insertvalue %{{.*}}, %{{.*}}[4, 1] : !llvm.struct<(ptr<f32>, ptr<f32>, i64, array<3 x i64>, array<3 x i64>)>
// CHECK: llvm.mul %{{.*}}, %{{.*}} : i64 // CHECK: llvm.mul %{{.*}}, %{{.*}} : i64
// CHECK: llvm.insertvalue %{{.*}}, %{{.*}}[4, 0] : !llvm.struct<(ptr<f32>, ptr<f32>, i64, array<3 x i64>, array<3 x i64>)> // CHECK: llvm.insertvalue %{{.*}}, %{{.*}}[4, 0] : !llvm.struct<(ptr<f32>, ptr<f32>, i64, array<3 x i64>, array<3 x i64>)>
// CHECK: llvm.mul %{{.*}}, %{{.*}} : i64 // CHECK: llvm.mul %{{.*}}, %{{.*}} : i64
// -----
// CHECK-LABEL: func @collapse_static_shape_with_unit_dims_with_dynamic_strides
func @collapse_static_shape_with_unit_dims_with_dynamic_strides(%arg: memref<1x1x8x8xf32, affine_map<(d0, d1, d2, d3)[s0] -> (d0 * 64 + s0 + d1 * 64 + d2 * 8 + d3)>>) -> memref<64xf32, affine_map<(d0)[s0] -> (d0 + s0)>> {
// CHECK-NOT: memref.collapse_shape
%1 = memref.collapse_shape %arg [[0, 1, 2, 3]] : memref<1x1x8x8xf32, affine_map<(d0, d1, d2, d3)[s0] -> (d0 * 64 + s0 + d1 * 64 + d2 * 8 + d3)>> into memref<64xf32, affine_map<(d0)[s0] -> (d0 + s0)>>
return %1 : memref<64xf32, affine_map<(d0)[s0] -> (d0 + s0)>>
}