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

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar added a comment.
Herald added a subscriber: liufengdb.

Apparently I forgot to hit submit (seeing unsubmitted comments below).



================
Comment at: mlir/docs/ShapeInference.md:28
+    constraining the input type to be tensor/vector elements or that the
+    elemental type be of a specific type (e.g., output of sign is of elemental
+    type `i1`) or class (e.g., float like).
----------------
rriddle wrote:
> Is `sign` a know thing we can link to?
Not really, I'm referring to basic math. I'll spell it out a bit more.


================
Comment at: mlir/include/mlir/Analysis/InferTypeOpInterface.h:73
+  ShapeStorageT dims;
+  bool ranked;
+  Type elementType;
----------------
rriddle wrote:
> nit: Can you pack this bool with either the type or the attribute?
Maybe, but it doesn't belong there logically: it matches dims and not the others. I'd prefer not pack it until needed (e.g., we could likely change how dims are stored here and then it could change the packing).


================
Comment at: mlir/include/mlir/Analysis/InferTypeOpInterface.h:86
+LogicalResult inferReturnTensorTypes(
+    LogicalResult (*componentTypeFn)(
+        MLIRContext *, Optional<Location> location, ValueRange operands,
----------------
rriddle wrote:
> Why not function_ref here? Also, is this function intended to be user facing? i.e. should other users just call this function?
Made function_ref and moved to namespace detail as I expect this to be called via inferReturnTypes, and this is just to extract common function out to reduce size of InferTensorType<...> below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72432