[PATCH] D78034: [mlir][spirv] Improve stride support in array types

[Lei Zhang via Phabricator via llvm-commits]

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


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h:157
   static ArrayType get(Type elementType, unsigned elementCount,
-                       LayoutInfo layoutInfo);
+                       unsigned stride);
 
----------------
mravishankar wrote:
> Seems to be a mismatch here. elementCount and stride should be size_t. Of course on many devices unsigned is enough, but when possible we might need to support objects of large sizes. Maybe better to make both size_t?
SGTM to be future proof. I used unsigned here to be consistent; better to change them together. (The serialization/deserialization side we need to be careful; the array stride is directly embedded as a literal there. I wonder whether all the drivers support non-32-bit cases nicely though. ;-P)


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp:231
+// array-type ::= `!spv.array` `<` integer-literal `x` element-type
+//                (`,` `stride` `=` integer-literal)? `>`
 static Type parseArrayType(SPIRVDialect const &dialect,
----------------
mravishankar wrote:
> This makes it too verbose. Why change the parsing/printing?
It makes the IR more readable. Otherwise if you have structs containing arrays, everything (struct member offsets, array strides) are represented as `[N]`. It's really hard to differentiate. And I typically think `[N]` is more naturally for indexing; thus good for offsets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78034