[PATCH] D79541: [mlir] Simplify and better document std.view semantics

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache marked 4 inline comments as done.
nicolasvasilache added inline comments.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:2910
   let description = [{
-    The "view" operation converts a 1-D memref with i8 element type,
-    to an N-D memref with arbitrary element type. In addition, the ViewOp
-    supports the following arguments:
-    *) A single dynamic offset operand can be specified which represents a
-       a dynamic offset within the base 1-D memref at which to create the
-       resulting memref view.
-    *) A dynamic size operand must be specified for each dynamic dimension
+    The "view" operation extracts an N-D contiguous memref (i.e. with empty or
+    identity layout map) with arbitrary element type from a 1-D contiguous
----------------
timshen wrote:
> "contiguous memref" -> "slice"? To me "contiguous" means "no gaps between the accessed bytes", but it doesn't imply identity layout. Coming from Python/C++ background, "slice" implies identity layout.
`slice` has potential conflicts in MLIR, there is a rank-reducing slice op in Linalg that is likely to be ported to std or to be folded within subview.

How about `"contiguous memref with empty or identity layout"` ?


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:2623
+    targetMemRef.setOffset(rewriter, loc,
+                           createIndexConstant(rewriter, loc, offset));
 
----------------
timshen wrote:
> If the offset is 0, would it work by not calling `setOffset()` at all?
If we don't set the offset we will have uninitialize memory that can be propagated further through subviews or function calls: the descriptor created here will be used in various ways and the offset will be used later in load/stores etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79541