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

Reply via email to