[PATCH] D72432: [mlir] Add shaped container component type interface

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Analysis/InferTypeOpInterface.h:22
+#include "mlir/IR/StandardTypes.h"
 #include "mlir/IR/Types.h"
 #include "mlir/Support/LLVM.h"
----------------
Can you remove Types.h now? Some of the others could also be trimmed a bit as well.


================
Comment at: mlir/include/mlir/Analysis/InferTypeOpInterface.h:37
+class ShapedTypeComponents {
+  // Internal storage type for shape.
+  using ShapeStorageT = SmallVector<int64_t, 3>;
----------------
// -> ///


================
Comment at: mlir/include/mlir/Analysis/InferTypeOpInterface.h:73
+  ShapeStorageT dims;
+  bool ranked;
+  Type elementType;
----------------
nit: Can you pack this bool with either the type or the attribute?


================
Comment at: mlir/include/mlir/Analysis/InferTypeOpInterface.h:86
+LogicalResult inferReturnTensorTypes(
+    LogicalResult (*componentTypeFn)(
+        MLIRContext *, Optional<Location> location, ValueRange operands,
----------------
Why not function_ref here? Also, is this function intended to be user facing? i.e. should other users just call this function?


================
Comment at: mlir/lib/Analysis/InferTypeOpInterface.cpp:17
 #include "mlir/IR/Attributes.h"
 #include "mlir/IR/Operation.h"
 #include "mlir/IR/Types.h"
----------------
Trim these headers?


================
Comment at: mlir/test/lib/TestDialect/TestDialect.cpp:318
+  shape.reserve(operands.size());
+  for (auto val : operands) {
+    if (auto sval = val->getType().dyn_cast<ShapedType>()) {
----------------
nit: operands.getTypes()


================
Comment at: mlir/test/lib/TestDialect/TestPatterns.cpp:72
+    for (int j = 0; j < e; ++j) {
+      std::vector<Value> values = {fop.getArgument(i), fop.getArgument(j)};
       SmallVector<Type, 2> inferedReturnTypes;
----------------
Can you use an array or inline these values instead?


================
Comment at: mlir/test/lib/TestDialect/TestPatterns.cpp:92
+      // Collect ops to avoid triggering on inserted ops.
+      for (auto &op : getFunction().getBody().front().getOperations())
+        ops.push_back(&op);
----------------
Drop the getOperations()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72432