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

[Jeff Niu via Phabricator via cfe-commits]

Mogball added inline comments.


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:211
+    // If `op` is not commutative, do nothing.
+    if (!op->template hasTrait<OpTrait::IsCommutative>())
+      return failure();
----------------
Mogball wrote:
> Please move the body `matchAndRewrite` into a source file. It only needs `Operation *`. And then all the structs/enum/utility functions in the header can be moved there as well.
This could stand to be a `static_assert`


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:257
+      // smallest.
+      DenseSet<unsigned> smallestKeyIndices;
+      // Stores the indices of the unassigned operands whose key is the largest.
----------------
Could you not change `getIndicesOfUnassigned...` to populate two lists of operands and pass these to `assignSortedPositionTo` instead of using a set to track the indices. You could put the operand index inside `OperandBFS` to keep track.


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:24
+bool mlir::hasAtLeastOneUnassignedOperand(
+    SmallVector<OperandBFS *, 2> bfsOfOperands) {
+  for (OperandBFS *bfsOfOperand : bfsOfOperands) {
----------------
This function doesn't seem like it pays for itself -- `llvm::any_of`?


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:92
+  // of `bfsOfOperands`.
+  SmallVector<mlir::BlockArgumentOrOpKey, 4> smallestKey, largestKey;
+  for (OperandBFS *bfsOfOperand : bfsOfOperands) {
----------------
These could all be `ArrayRef`s since you aren't modifying them


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:167
+      // which is the key associated with a block argument.
+      mlir::BlockArgumentOrOpKey blockArgumentOrOpKey;
+      blockArgumentOrOpKey.type = BLOCK_ARGUMENT;
----------------
drop mlir::


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:201
+                                  unsigned positionToAssign, Operation *op) {
+  if (keyIndices.contains(indexOfOperand) &&
+      (isTheOnlyKey || bfsOfOperand->ancestorQueue.empty())) {
----------------
This function doesn't seem like it pays for itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124750