[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D124750#3502228 <https://reviews.llvm.org/D124750#3502228>, @srishti-pm wrote:

> In D124750#3500748 <https://reviews.llvm.org/D124750#3500748>, @mehdi_amini wrote:
>
>> Seems like this should be added to canonicalization? The "push constants to the right hand side" is there already.
>
> I think this was not added to canonicalization because we wanted it to be an independent utility that can be used if needed and not be used if not needed.

You're telling me "what" while I'm actually more interested in the "why" here?

>> I also don't understand the complexity of the implementation, I may need an example to understand why you're recursively operating on the producer ops for the operands.
>> From the revision description: (1) the operands defined by non-constant-like ops come first, followed by (2) block arguments, and these are followed by (3) the operands defined by constant-like ops` which all seems like a fairly local check: a stable-sort on the operands would be deterministic and local to a single operation.
>
> I do this because, firstly, in the description, if you look below this paragraph, you will see the following:
> "And, if two operands come from the same op, the function backtracks and
> looks even further to sort them. This backtracking is done over the
> backward slice of the operand, in a breadth-first traversal."

Same as before: this does not tell me why, can you provide an example where this matters?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124750