[PATCH] D79759: [mlir] Add SubViewOp::getOrCreateRanges and fix folding pattern

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:2493
 
-SmallVector<SubViewOp::Range, 8> SubViewOp::getRanges() {
+static Value getOrCreateSubViewPart(OpBuilder &b, Location loc,
+                                    ValueRange values, ArrayAttr attr,
----------------
Please add documentation.


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:2503
+  unsigned numDynamicEntriesUpToIdx = std::count_if(
+      attr.getValue().begin(), std::prev(attr.getValue().begin() + idx),
+      [&](Attribute attr) {
----------------
I don't understand why you do std::prev, and there is no comment to help. Normally, all standard algorithms exclude the upper bound, so this would exclude the element _before_ idx-th.


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:2709
 
-OpFoldResult SubViewOp::fold(ArrayRef<Attribute>) {
-  auto folds = [](Operation *op) {
-    bool folded = false;
-    for (OpOperand &operand : op->getOpOperands()) {
-      auto castOp = operand.get().getDefiningOp<MemRefCastOp>();
-      if (castOp && canFoldIntoConsumerOp(castOp)) {
-        operand.set(castOp.getOperand());
-        folded = true;
-      }
+/// Pattern to rewrite a subview op with MemRefCast arguments.
+class SubViewOpMemRefCastFolder final : public OpRewritePattern<SubViewOp> {
----------------
Could you please explain _how_ is it rewritten?


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:2714
+
+  LogicalResult matchAndRewrite(SubViewOp subViewOp,
+                                PatternRewriter &rewriter) const override {
----------------
This needs a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79759