[PATCH] D79662: [mlir] Revisit std.subview handling of static information.

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added inline comments.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:2561
+      "ValueRange offsets, ValueRange sizes, ValueRange strides, "
+      "ArrayRef<int64_t> staticOffsets = ArrayRef<int64_t>(), "
+      "ArrayRef<int64_t> staticSizes = ArrayRef<int64_t>(), "
----------------
mravishankar wrote:
> AFAIK the verifier code below, the staticOffsets, staticSizes and staticStrides need to be specified always. So having a default value here is misleading. Can we move these operands before the offsets, sizes and strides (I know that makes it a breaking change)
I can split this in 2 builders, one that needs to take everything and another one that takes none and documents that it uses the default which is "all dynamic".
Clients shouldn't have to worry about passing always dynamic entries for everything (which is the way one would almost always construct this and later call canonicalization).


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:2341
+  // Fill with the appropriate dynamic marker.
+  if (staticOffsetsVector.empty())
+    staticOffsetsVector.assign(rank, ShapedType::kDynamicStrideOrOffset);
----------------
mravishankar wrote:
> I find this a bit misleading. If left empty in the build, then this implicitly means that all the values are -1. I would rather be explicit that the subview needs an ArrayAttr for staticOffsets, staticSizes, staticStrides that is of size rank of the source. Based on what are -1, will dictate the sizes of offsets, sizes and strides that are specified. This would make the dynamic values optional and needed only if one of the static* has a -1 entry. Note this is more a suggestion.
very good point, addressed with 2 builders.


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:2390
+    res.push_back(attr.getInt());
+  return res;
 }
----------------
mravishankar wrote:
> isnt this just
> 
> llvm::to_vector<4>(llvm::map_range(attr, [](Attribute a) -> int64_t { return a.cast<IntegerAttr>().getInt();}))
yes, thanks, much cleaner!


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:2508-2509
+    SmallVector<int64_t, 8> newStaticOffsets =
+        extractFromI64ArrayAttr(subViewOp.static_offsets());
+    assert(newStaticOffsets.size() == subViewOp.getRank());
+    canonicalizeSubViewPart(newOffsets, newStaticOffsets,
----------------
mravishankar wrote:
> These two lines can be moved into the canonicalizeSubViewPart method since they are repeated below.
I need to pass a preallocated vector that can be modified in place. 
I could also return 2 values but it wouldn't change anything I'd claim except I'd have to return a pair and std::tie.
And like people will complain that they want a struct instead of a pair so at this point, you know :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79662