Page MenuHomePhabricator

Add a pattern to combine composed subview ops
ClosedPublic

Authored by agrue on Tue, Mar 23, 5:08 PM.

Diff Detail

Event Timeline

agrue created this revision.Tue, Mar 23, 5:08 PM
agrue requested review of this revision.Tue, Mar 23, 5:08 PM
agrue updated this revision to Diff 333093.Wed, Mar 24, 12:44 PM

Fix relevant CMakeLists.txt

mehdi_amini added inline comments.Thu, Mar 25, 12:09 AM
mlir/lib/Dialect/StandardOps/Transforms/ComposeSubView.cpp
77

FYI LLVM is still using C++14, this won't build with all of our supported platforms unfortunately (it is neat though!)

agrue updated this revision to Diff 333409.Thu, Mar 25, 1:30 PM

Fix clang-tidy warnings

mravishankar requested changes to this revision.Sat, Mar 27, 3:02 PM
mravishankar added inline comments.
mlir/lib/Dialect/StandardOps/Transforms/ComposeSubView.cpp
69

Not sure about the math for strides != 1. It can have weird corner cases (I am not saying it doesnt work, or I have a specific flaw I see in the code here, but just an intuition). Unless really needed, maybe restrict to strides = 1 case in source and dest subviews?

117

If one of the offsets is a constant then you can construct the affine map with the constant already. So do something like this

AffineExpr expr;
SmallVector<Value> operands;
auto addValueOrAttr = [&](OpFoldResult valueOrAttr) {
  if (auto attr = valueOrAttr.dyn_cast<Attribute>()) {
    expr = expr + attr.cast<IntegerAttr>().getInt();
  } else {
    expr = expr + getAffineSymbolExpr(operands.size());
    operands.push_back(valueOrAttr.get<Value>());
  }
};
AffineMap map = AffineMap::get(0, operands.size(), expr);
This revision now requires changes to proceed.Sat, Mar 27, 3:02 PM
agrue updated this revision to Diff 334326.Tue, Mar 30, 7:36 PM

Address review comments

agrue marked 3 inline comments as done.Wed, Mar 31, 9:22 AM
agrue added inline comments.
mlir/lib/Dialect/StandardOps/Transforms/ComposeSubView.cpp
69

Makes sense. We only need strides == 1. Done.

117

I see, thanks. Done.

mravishankar requested changes to this revision.Wed, Mar 31, 11:25 AM

Thanks. Just a few minor nits, this time around.

mlir/lib/Dialect/StandardOps/Transforms/ComposeSubView.cpp
69

Just to make it cleaner. Just add the strides == 1 check outside of the loop. You dont need to iterate over it here. Makes it simpler to read. Also you can just create the strides for the result later on.

85

All the stride stuff is better checked before. You just need this check

if (llvm::any_of(strides, [](OpFoldResult &valueOrAttr) {
  Attribute attr = valueOrAttr.dyn_cast<Attribute>();
  return attr && attr.cast<IntegerAttr>().getInt() == 1;
}

So you ca

98

Nit: I think you can drop the if here and get result below using

Value result = affineMapToValues(rewriter, loc, AffineMap::get(0, affineApplyOperands.size(), expr), affineApplyOperands)[0];

https://github.com/llvm/llvm-project/blob/fe5c4a06a49c697399323cc55e2135a5452ed616/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h#L28

More readable (to me).

This revision now requires changes to proceed.Wed, Mar 31, 11:25 AM
mravishankar accepted this revision.Wed, Mar 31, 2:21 PM

LGTM-ing this based on the strides being checked ahead of time (instead of in the loop)

mlir/lib/Dialect/StandardOps/Transforms/ComposeSubView.cpp
98

Dropping this for now.

This revision is now accepted and ready to land.Wed, Mar 31, 2:21 PM
agrue updated this revision to Diff 334548.Wed, Mar 31, 3:07 PM
agrue marked 2 inline comments as done.

Address further review comments.

agrue marked 4 inline comments as done.Wed, Mar 31, 3:09 PM

Thanks. Can someone else please land this patch when it's ready? I don't have permissions.

This revision was automatically updated to reflect the committed changes.

Why it is located in MLIRStandardOpsTransforms, while operating in MemRef dialect? I would expect it to be a part of MLIRMemRefTransforms.