[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#3502401 <https://reviews.llvm.org/D124750#3502401>, @srishti-pm wrote:

> In D124750#3502295 <https://reviews.llvm.org/D124750#3502295>, @mehdi_amini wrote:
>
>> You're telling me "what" while I'm actually more interested in the "why" here?
>
> I'm not sure what your question is, with a "why". Let me think about this a bit. I'll get back to you.

My "why?" question is about canonicalization: could this be a canonicalization and if so why / why not? This is an important thing to answer before looking into the discussion below actually:

>> Same as before: this does not tell me why, can you provide an example where this matters?
>
> Sure. This is a bit lengthy. I'm really sorry for that !

No worry, thanks for being thorough.

> ...
> So, in essence, we can see that the effort of an end user writing a C++ pattern has reduced if I sort the producers as well.

So far you explained "what is canonicalization and why are we canonicalizing", the same rationale applies to "push constant to the right" that we do already in canonicalization, and this is exactly why I asked before whether we could do what you're doing as a canonicalization.

> The real reason for sorting the producers is that, if such a thing is not done, the sorting and this entire utility will be virtually useless.

So: I understand that the producers should be sorted for a pattern to apply, **but** our disconnect earlier is that my usual approach to see canonicalization is to process an entire block/region, and as such we don't work on slices but on operation in order until fix-point. I'm a bit concerned about efficiency of your approach, because when integrated in a framework that actually visit the entire block you would recompute subset of the same slice over and over and re-attempt to sort everything multiple times:

> d = add b, c
> s = add d, f
> g = add s, d
> h = add d, x
> i = add g, h

Every time the algorithm would consider a commutative op, that is all the op, it would recurse and try to re-sort the current slice, processing the same ops over and over.

> A deterministic sorting of an op requires its producers to be sorted.

This isn't clear to me why based on the sorting criteria you provided, but in any case a local sort with fixed-point iteration on an isolated region should converge (hopefully).

> Our sorting algorithm is based on the breadth-first traversal of backard slices. On the same level of DAG, the traversal looks at operands from the first to the last. That is how the breadth-first traversal is defined here. Now, if this traversal is non-deterministic, then, the whole use of sorting something will collapse. **Maybe, this can be BEST explained with the below example.**
>
> If I have this IR:
> d = div b, c
> s = sub e, f
> x = xor k, l
> g = add s, d
> h = add d, x
> i = add g, h
>
> Then, `i = add g, h` will be sorted to `i = add g, h` (no change).
>
> **But**, when I have the below IR (which is functionally the same as the above IR):
> d = div b, c
> s = sub e, f
> x = xor k, l
> g = add d, s
> h = add d, x
> i = add g, h
>
> Then, `i = add g, h` will be sorted to `i = add h, g`.

Why? Again your description of the sort is as follow:

> (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. 
> In addition to this, within the category (1), the order of operands is alphabetical w.r.t. the dialect name and op name.

In your example:

  g = add d, s
  h = add d, x
  i = add g, h

The operands fits category 1) I believe, and so they should be sorted "alphabetical w.r.t. the dialect name and op name", so a stable sort would never re-order them in any way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124750