[PATCH] D106422: Implement recursive support into OperationEquivalence::isEquivalentTo()

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added a comment.
This revision is now accepted and ready to land.

LGTM

(Don't forget to resolve the STLExtras diffbase first)



================
Comment at: mlir/lib/IR/OperationSupport.cpp:551
+  Optional<bool> allBlockMatch =
+      llvm::all_of_zip(*lhs, *rhs, [&](Block &lBlock, Block &rBlock) {
+        // Check block arguments.
----------------
Can you pull the functors out as separate variables to reduce indent here?

```
auto allOpsMatchFn = ...;
auto allBlockMatchFn = ...;
return llvm::all_of_zip(*lhs, *rhs, allBlockMatchFn) == true;
```


================
Comment at: mlir/lib/IR/OperationSupport.cpp:593
+            });
+        return allOpsMatch.hasValue() && allOpsMatch.getValue();
+      });
----------------
Same as below.


================
Comment at: mlir/lib/IR/OperationSupport.cpp:595
+      });
+  return allBlockMatch.hasValue() && allBlockMatch.getValue();
+}
----------------
?

Or `allBlockMatch.getValueOr(false)`


================
Comment at: mlir/lib/IR/OperationSupport.cpp:618-633
+    Value curArg = std::get<0>(operandPair);
+    Value otherArg = std::get<1>(operandPair);
+    if (curArg.getType() != otherArg.getType())
+      return false;
+    if (failed(mapOperands(curArg, otherArg)))
+      return false;
+  }
----------------
Would a lambda that removed the duplication here be cleaner?


================
Comment at: mlir/lib/Transforms/CSE.cpp:32-33
+    return OperationEquivalence::computeHash(
+        const_cast<Operation *>(opC), [](Value v) { return hash_value(v); },
+        [](Value v) { return llvm::hash_code{}; },
+        OperationEquivalence::IgnoreLocations);
----------------
Can you add param comments?

Also, is it possible to just pass hash_value directly for the operand hash? Or do you need to wrap it?


================
Comment at: mlir/lib/Transforms/CSE.cpp:44-52
+    auto eq = OperationEquivalence::isEquivalentTo(
+        const_cast<Operation *>(lhsC), const_cast<Operation *>(rhsC),
+        [](Value lhs, Value rhs) { return success(lhs == rhs); },
+        [](Value lhs, Value rhs) { return success(); },
+        OperationEquivalence::IgnoreLocations);
+    llvm::errs() << "Equal: " << *const_cast<Operation *>(lhsC) << " vs "
+                 << *const_cast<Operation *>(rhsC) << " compares " << eq
----------------
I feel like we should have a util for the default comparison/hash as well as the ignore case.


================
Comment at: mlir/test/Transforms/cse.mlir:267
   return
-} 
+}
----------------
Unrelated?


================
Comment at: mlir/test/lib/IR/TestOperationEquals.cpp:28-29
+                         << opCount;
+      signalPassFailure();
+      return;
+    }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106422