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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:2922
+    requires the op to have special semantics because, e.g. an offset of 3 * i8*
+    cannot be represented as an offset on f64*.
+    In order to simplify implementations and to separate concerns, a "view" op:
----------------
`3 * i8` is what you mean here I think? I read what you wrote as "3 x pointers-size"


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:2932
+    function boundaries: the offset from the allocated pointer is not available
+    in the returned view. Aliasing information is expected to be extracted and
+    passed as operand attributes at function boundaries. Note that this would
----------------
The part about function boundaries and "the offset from the allocated pointer is not available in the returned view." isn't clear to me. In particular this seems to be referring to some lowering specific aspects.



================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:2936
+    allocated value is necessary to resolve aliasing, in the absence of
+    attribute information.
+    If the offset behavior above is not sufficient, the alternatives are:
----------------
All the part about aliasing isn't clear to me either, it would benefit from examples of the attributes you refer to here.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:2941
+      2) change memref layout semantics.
+    Both options are seen as unnecessarily intrusive and complex at this time.
+
----------------
I don't know what this whole paragraph intends to convey, but I'm not sure it really describes the operation anyway.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:2950
     // ViewOp with static offset and sizes.
     %1 = view %0[][] : memref<2048xi8> to memref<64x4xf32>
 
----------------
Seems invalid now.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:2954
     %2 = view %0[%offset_1024][%size0]
       : memref<2048xi8> to memref<?x4xf32, (d0, d1)[s0] -> (d0 * 4 + d1 + s0)>
 
----------------
The returned memref has non-identity map which you forbid in the description above.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:2981
+    /// Returns the dynamic sizes for this view operation. This is redundant
+    /// with `sizes` but needed in template implementations.
     operand_range getDynamicSizes() {
----------------
What kind of template implementations (instantiations?) require this?


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:2599
       return op->emitWarning("cannot cast to non-strided shape"), failure();
+    assert(offset == 0 && "expected offset to be 0");
 
----------------
The doc says that the returned memref has `0 offset and contiguous layout  (i.e. empty or identity layout map).`
Seems like this code above isn't useful, am I misunderstanding that this will always look into an identity map? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79541