[PATCH] D73296: [mlir] Add MemRefTypeBuilder and refactor some MemRefType::get().

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.

Thanks, I like the simplification.

Note there's one broken test (clang ones are irrelevant).



================
Comment at: mlir/include/mlir/IR/StandardTypes.h:442
 
+class MemRefTypeBuilder {
+  ArrayRef<int64_t> shape;
----------------
Could you please add documentation to this class? Especially as it captures (through ArrayRef) references to data that must be kept live at least as long as this object.


================
Comment at: mlir/include/mlir/IR/StandardTypes.h:446
+  ArrayRef<AffineMap> affineMaps;
+  unsigned memorySpace;
+
----------------
Nit: we tend to put private members after public


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:580
+                       .setShape(sizes)
+                       .setAffineMaps({makeStridedLinearLayoutMap(
+                           strides, offset, b->getContext())})});
----------------
Drop {} inside setAffineMaps (and also addTypes), ArrayRef<T> is implicitly constructible from T.


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:581
+                       .setAffineMaps({makeStridedLinearLayoutMap(
+                           strides, offset, b->getContext())})});
 }
----------------
Suggestion: since you are adding a helper class already, can you integrate `makeStidedLinearLayoutMap` into it directly as `MemRefTypeBuilder::setLinearLayoutMap`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73296