[PATCH] D72559: [mlir] Generalize broadcastable trait operands

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Traits.h:83
 
+// TODO: Remove post updating call sites.
+template <typename ConcreteType>
----------------
antiagainst wrote:
> 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.
Good point, remvoed. We don't want a new trait, the intention is to use two existing traits instead that define the orthogonal constraints.

Seems we had fixed the bug but not the documentation.


================
Comment at: mlir/include/mlir/IR/OpBase.td:1333
+def Broadcastable :
+  NativeOpTrait<"BroadcastableTwoOperandsOneResultWithSameElementType">;
+// Op supports operand broadcast behavior.
----------------
mehdi_amini wrote:
> jpienaar wrote:
> > mehdi_amini wrote:
> > > To me `Broadcastable` implying element type seem fairly intuitive, have you looked into introducing a "BroadcastableDimensions" instead?
> > Broadcastable is only about shape* not element type: comparison operators have broadcasting behavior but boolean element type. Broadcastable should be the base and if we want to specialize it, it should get further constraints (e.g., BroadcastableWithSameElement is specialization of Broadcastable and clear from the name) rather than have specializations be more generic (e.g., BroadcastableDimensions is not Broadcastable).
> > 
> > * https://docs.scipy.org/doc/numpy/reference/ufuncs.html#broadcasting
> The Numpy link is using terminology like ` “broadcastable” to the same shape`. I'd still like an explicit name to avoid confusion and possible error by misuse: what about `ShapeBroadcastable`? 
Renamed to ResultsBroadcastableShape to emphasize that it affects the result (not just the operands as could have been the case) and is about shape. I might want to spell this differently still in future or add variant (e.g., certain operands can affect certain outputs, so while this is a convenient attribute for a common case, it is for special case and we might want to generalize it).

Open to alternatives :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72559