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