Page MenuHomePhabricator

[mlir] Generalize broadcastable trait operands
ClosedPublic

Authored by jpienaar on Jan 11 2020, 10:06 AM.

Details

Summary

Generalize broadcastable trait to variadic operands. Update the documentation that still talked about element type as part of broadcastable trait (that bug was already fixed). Also rename Broadcastable to ResultBroadcastableShape to be more explicit that the trait affects the result shape (it is possible for op to allow broadcastable operands but not have result shape that is broadcast compatible with operands).

Doing some intermediate work to have getBroadcastedType take an optional elementType as input and use that if specified, instead of the common element type of type1 and type2 in this function.

Diff Detail

Event Timeline

jpienaar created this revision.Jan 11 2020, 10:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2020, 10:06 AM
mehdi_amini added inline comments.Jan 11 2020, 10:28 AM
mlir/include/mlir/IR/OpBase.td
1333

To me Broadcastable implying element type seem fairly intuitive, have you looked into introducing a "BroadcastableDimensions" instead?

Unit tests: pass. 61655 tests passed, 0 failed and 777 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

jpienaar marked an inline comment as done.Jan 13 2020, 7:54 AM
jpienaar added inline comments.
mlir/include/mlir/IR/OpBase.td
1333

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).

antiagainst added inline comments.Jan 15 2020, 6:05 AM
mlir/include/mlir/Dialect/Traits.h
56

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."

80

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.

mehdi_amini added inline comments.Jan 16 2020, 10:44 PM
mlir/include/mlir/IR/OpBase.td
1333

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?

jpienaar updated this revision to Diff 238990.Jan 19 2020, 7:43 AM
jpienaar marked 4 inline comments as done.

Update summary as I realized the doc was out of date but we had already fixed the bug about element type. Instead generalized broadcastable to variadic operands, rename trait and addressed comments.

jpienaar retitled this revision from [mlir] Add trait for staging update of broadcastable trait to [mlir] Generalize broadcastable trait operands.Jan 19 2020, 7:44 AM
jpienaar edited the summary of this revision. (Show Details)
jpienaar added a reviewer: mehdi_amini.
jpienaar added inline comments.Jan 19 2020, 7:49 AM
mlir/include/mlir/Dialect/Traits.h
80

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.

mlir/include/mlir/IR/OpBase.td
1333

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 :)

Unit tests: pass. 61999 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

mehdi_amini accepted this revision.Jan 19 2020, 9:54 AM
This revision is now accepted and ready to land.Jan 19 2020, 9:54 AM
This revision was automatically updated to reflect the committed changes.