This is an archive of the discontinued LLVM Phabricator instance.

Add a pattern to combine composed subview ops
ClosedPublic

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

Diff Detail

Event Timeline

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

Fix relevant CMakeLists.txt

mehdi_amini added inline comments.Mar 25 2021, 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.Mar 25 2021, 1:30 PM

Fix clang-tidy warnings

mravishankar requested changes to this revision.Mar 27 2021, 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.Mar 27 2021, 3:02 PM
agrue updated this revision to Diff 334326.Mar 30 2021, 7:36 PM

Address review comments

agrue marked 3 inline comments as done.Mar 31 2021, 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.Mar 31 2021, 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.

84

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

97

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.Mar 31 2021, 11:25 AM
mravishankar accepted this revision.Mar 31 2021, 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
97

Dropping this for now.

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

Address further review comments.

agrue marked 4 inline comments as done.Mar 31 2021, 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.