[PATCH] D79541: [mlir] 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:2919
 
+    The "view" operation gives a structured indexing form to a flat 1-D buffer.
+    Unlike "subview" it can perform a type change. The type change behavior
----------------
mravishankar wrote:
> There is another problem w.r.t to view operation w.r.t to lowering to SPIR-V dialect. This is doing a type-cast. The SPIR-V spec is very restrictive in pointer casts it allows to the effect that current plan is to not use pointer casts at all within the SPIR-V dialect. While it is a SPIR-V restriction, I am not sure what the type casting here gives us. Specifically this is always type-casting (cause the base memref is i8). The requirement of creating a multi-dimension memref can be done using an AllocOp directly. If the need is to model aliasing between different uses you can use a memref_cast. The memref_cast can also be done for changing element type. That would then be an "opt-in". You do it only when you need it. For now I am not sure whether we need the ViewOp at all.
There are a few moving pieces here but the use case this wants to serve is a single bigger alloc from which multiple smaller pieces are extracted, alias or not and memory chunks that can be reused with linear allocation schemes.

Any op that casts from a small type to a bigger type will have similar issues if it wants to shift the base address by quantities that are not multiples of the bigger type size. Then there will also be alignment considerations and other fun stuff.

Re. using the op in particular scenarios I view this as a separate concern from the fact that the implementation of the op did not match the semantics and that the semantics and description needed to be updated to distinguish between offset and byte_shift. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79541