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

[Jeff Niu via Phabricator via cfe-commits]

Mogball added a comment.

I haven't thought too hard about the algorithm itself yet. I'm in the camp of "let's move forward if it works". I have mostly trivial comments.



================
Comment at: clang/docs/tools/clang-formatted-files.txt:8451
 mlir/lib/Transforms/SymbolPrivatize.cpp
+mlir/lib/Transforms/Utils/CommutativityUtils.cpp
 mlir/lib/Transforms/Utils/ControlFlowSinkUtils.cpp
----------------
I don't think this file needs to be modified.


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:24
+/// Declares various types of operations and block argument.
+enum BlockArgumentOrOpType {
+  /// Pertains to a block argument.
----------------
Why do all of these need to be exposed publicly? I think this file should only contain `SortCommutativeOperands`.


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:34
+/// Stores the "key" associated with a block argument or an operation.
+struct BlockArgumentOrOpKey {
+  /// Holds `BLOCK_ARGUMENT`, `NON_CONSTANT_OP`, or `CONSTANT_OP`, depending on
----------------
`using BlockArgumentOrOpKey = std::pair<BlockArgumentOrOpType, StringRef>`

The default `operator<` for `std::pair` should work for you.


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:140
+/// operand refers to one which has not been assigned a sorted position yet.
+bool hasAtLeastOneUnassignedOperand(SmallVector<OperandBFS *, 2> bfsOfOperands);
+
----------------
`ArrayRef<OperandBFS *>`


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:161
+/// (C) `secondKey` < `firstKey` condition is defined likewise.
+int compareKeys(SmallVector<BlockArgumentOrOpKey, 4> firstKey,
+                SmallVector<BlockArgumentOrOpKey, 4> secondKey);
----------------
Pass these both by `ArrayRef`


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:174
+void getIndicesOfUnassignedOperandsWithSmallestAndLargestKeys(
+    SmallVector<OperandBFS *, 2> bfsOfOperands,
+    DenseSet<unsigned> &smallestKeyIndices,
----------------
`ArrayRef<OperandBFS *>`


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:186
+/// progression of the traversal.
+void updateKeys(SmallVector<OperandBFS *, 2> bfsOfOperands);
+
----------------
`ArrayRef<OperandBFS *>`


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:194
+                            DenseSet<unsigned> keyIndices, bool isTheOnlyKey,
+                            SmallVector<Value, 2> &sortedOperands,
+                            unsigned positionToAssign, Operation *op);
----------------
`SmallVectorImpl`


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:201
+void popFrontAndPushAdjacentUnvisitedAncestors(
+    SmallVector<OperandBFS *, 2> bfsOfOperands);
+
----------------
`ArrayRef<OperandBFS *>`


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:211
+    // If `op` is not commutative, do nothing.
+    if (!op->template hasTrait<OpTrait::IsCommutative>())
+      return failure();
----------------
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.


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:226
+    for (Value operand : op->getOperands()) {
+      OperandBFS *bfsOfOperand = new OperandBFS();
+      bfsOfOperand->pushAncestor(operand.getDefiningOp());
----------------
memory leak?


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:242
+    // At first, all elements in it are initialized as null.
+    SmallVector<Value, 2> sortedOperands;
+    for (unsigned i = 0; i < numOperands; i++)
----------------
`sortedOperands(numOperands, nullptr);`


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:275
+      // sorting).
+      for (auto indexedBfsOfOperand : llvm::enumerate(bfsOfOperands)) {
+        OperandBFS *bfsOfOperand = indexedBfsOfOperand.value();
----------------



================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:296
+      // to assign it a sorted position if possible (ensuring stable sorting).
+      for (auto indexedBfsOfOperand :
+           llvm::enumerate(llvm::reverse(bfsOfOperands))) {
----------------



================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:17
+
+#define DEBUG_TYPE "commutativity-utils"
+
----------------
unused?


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:51
+/// (C) `secondKey` < `firstKey` condition is defined likewise.
+int mlir::compareKeys(SmallVector<mlir::BlockArgumentOrOpKey, 4> firstKey,
+                      SmallVector<mlir::BlockArgumentOrOpKey, 4> secondKey) {
----------------
The doc of a public function shouldn't be repeated above the implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124750