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

[Nicolas Vasilache via Phabricator via llvm-commits]

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


================
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
----------------
mehdi_amini wrote:
> 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.
> 
Dropping this part as it is not strictly related to this op's semantics.


================
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:
----------------
mehdi_amini wrote:
> All the part about aliasing isn't clear to me either, it would benefit from examples of the attributes you refer to here.
Dropping this part as it is not strictly related to this op's semantics.


================
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.
+
----------------
mehdi_amini wrote:
> I don't know what this whole paragraph intends to convey, but I'm not sure it really describes the operation anyway.
Dropping this part as it is not strictly related to this op's semantics.


================
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() {
----------------
mehdi_amini wrote:
> What kind of template implementations (instantiations?) require this?
Added as a comment.


================
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");
 
----------------
mehdi_amini wrote:
> 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? 
It does extract the mixed static-dynamic strides from the sizes + identity layout.
The strides are used below to fill the descriptor.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79541