[Rewriter] Extend list of supported commutative operations #2741
Conversation
Signed-off-by: Christoph Berganski <christoph.berganski@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2741 +/- ##
=======================================
Coverage 70.09% 70.09%
=======================================
Files 226 226
Lines 27388 27389 +1
Branches 2781 2781
=======================================
+ Hits 19198 19199 +1
Misses 7234 7234
Partials 956 956 ☔ View full report in Codecov by Sentry. |
|
@gramalingam do you see potential performance impact with this change? |
| return len(self._outputs) | ||
|
|
||
| def commute(self) -> Sequence[GraphPattern]: | ||
| # List all commutative elementwise (binary) operators for which we |
There was a problem hiding this comment.
This is fine ... but, please note that the existing implementation can lead to an exponential increase in the number of patterns that are checked, with efficiency implications. (The underlying implementation needs to be improved: for example, now that there is support for disjunctive (OR) patterns, it may be useful to use that instead of creating many patterns.)
There was a problem hiding this comment.
Should we make commute rewrite the pattern into disjunctive patterns instead?
Note: This tracks the not-yet-merged extended commutativity pattern PR, see microsoft/onnxscript#2741
3 participants
No description provided.