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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.
Herald added 1 blocking reviewer(s): rriddle.


================
Comment at: mlir/include/mlir/IR/StandardTypes.h:442
 
+class MemRefTypeBuilder {
+  ArrayRef<int64_t> shape;
----------------
ftynse wrote:
> 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.
I'm generally a bit apprehensive about these types of builders(as it doesn't really align with the rest of the type infra), but in this case it seems to have some value in the simplification it brings. A few comments:

* I would prefer to move this class inside of MemRefType and out of the global mlir namespace.
(i.e. MemRefTypeBuilder -> MemRefType::Builder)

* This is missing quite a bit of documentation.



================
Comment at: mlir/include/mlir/IR/StandardTypes.h:444
+  ArrayRef<int64_t> shape;
+  Type elementType;
+  ArrayRef<AffineMap> affineMaps;
----------------
nit: Structure classes with public members first and private last:

public:
...

private:
...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73296