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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
