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

Seems like this should be added to canonicalization? The "push constants to the right hand side" is there already.

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.



================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:235
+      bfsOfOperand->key = (Twine(bfsOfOperand->key) + Twine("1") +
+                           std::string(frontAncestor->getName().getStringRef()))
+                              .str();
----------------
The string conversion seems unnecessary to me?


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:271
+  for (Operation *operandDefOp : operandDefOps) {
+    OperandBFS *bfsOfOperand = new OperandBFS();
+    bfsOfOperand->pushAncestor(operandDefOp);
----------------
Is this a leak?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124750