[PATCH] D72559: [mlir] Add trait for staging update of broadcastable trait

[Lei Zhang via Phabricator via llvm-commits]

antiagainst added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Traits.h:56
+/// If the elementType is not provided, then this requires that the element type
+/// of type1 and type2 matches and is the same as the result type.
+Type getBroadcastedType(Type type1, Type type2, Type elementType = nullptr);
----------------
This is a bit counter-intuitive to me: the result type is naturally the same as `type1` and `type2` if not explicitly providing an `elementType`. What about: "If `elementType` is not provided, this requires `type1` and `type2` to have the same element type; otherwise, the given `elementType` will be used as the result's element type, ignoring element types from `type1` and `type2`."


================
Comment at: mlir/include/mlir/Dialect/Traits.h:83
 
+// TODO: Remove post updating call sites.
+template <typename ConcreteType>
----------------
The current implementation of BroadcastableTwoOperandsOneResult only checks shape. I don't quite understand why don't we add a new trait to built on the existing trait to further constraint the element type if we want BroadcastableTwoOperandsOneResultWithSameElementType.

I do remember there are use cases in TensorFlow relying on `BroadcastableTwoOperandsOneResult` being not checking element types... This is gonna cause problem for those cases if we remove this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72559