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

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar added a comment.

The change looks fine to me. As mentioned in commit message, some of the uses need to be re-evaluated (the one im hitting is the use of view in memory promotion)



================
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
----------------
nicolasvasilache wrote:
> 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. 
> 
Thanks for clarification.  The view op as stated itself could be lowered to SPIR-V, but the issue is the backing allocation. The allocation type will have to be "deduced" bsaed on its type, and then fail if there are multiple uses with different types. Thats an OK-ish solution, but not a clean solution for sure. Maybe the uses of view op can be looked into and avoided (in memory promotion for example).
No further comments on this now. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79541