This allows to use OperationEquivalence to track structural comparison for equality between two
Operations.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/IR/Region.cpp | ||
---|---|---|
197 ↗ | (On Diff #360361) | So this requires all ops in the same order independent of whether has SSA dominance or not, This equal is identically equal including verifying non-semantic changing differences? |
Nit on description, this adds functionality and doesn't feel appropriate marked as NFC which is normally reserved for refactoring/cleanup.
mlir/lib/IR/Region.cpp | ||
---|---|---|
196 ↗ | (On Diff #360361) | We have pretty large modules with some growing in nesting of regions depth. |
mlir/lib/IR/Region.cpp | ||
---|---|---|
197 ↗ | (On Diff #360361) | (only exception I think is locations, locations may differ) |
mlir/lib/IR/Region.cpp | ||
---|---|---|
196 ↗ | (On Diff #360361) | You're commenting on the recursive aspect of the algorithm? At the moment, we tolerate it for the two following cases: - The nesting of the IR: we use recursion when traversing nested regions. - Type nesting: recursion may be used for the nesting of composite types. Do you think that it is time to revisit this? I always assumed that nesting was fairly bounded (i.e. ~20 would be close to the max we expect to see anytime soon?). |
197 ↗ | (On Diff #360361) | Oh I forgot locations, I should check on this as well (maybe behind an option). And yes this is really "equality" and not "equivalence" in any way. |
mlir/include/mlir/IR/Operation.h | ||
---|---|---|
214 ↗ | (On Diff #360630) | I'm not sure I understand the distinction between this and the existing OperationEquivalence utils. Can you explain what purpose this serves that the other doesn't/can't? Or why we shouldn't just remove the OperationEquivalence stuff in favor of this? From a glance they serve identical purposes. |
mlir/lib/IR/Region.cpp | ||
169 ↗ | (On Diff #360630) | Cache the end iterators to avoid recomputing them. |
mlir/include/mlir/IR/Operation.h | ||
---|---|---|
214 ↗ | (On Diff #360630) | Ah! I just forgot about OperationEquivalence existence. |
mlir/include/mlir/IR/Operation.h | ||
---|---|---|
214 ↗ | (On Diff #360630) | I originally added just enough to support the basic CSE stuff, but the intention was to expand upon it when that became necessary. Should we remove it and wrap everything from it into this equals? I don't have a preference which one we expand, but I don't think we need both. |
mlir/include/mlir/IR/Operation.h | ||
---|---|---|
214 ↗ | (On Diff #360630) | Since OperationEquivalence supports computing hash, seems like I'll consolidate everything there! |
mlir/test/lib/IR/TestOperationEquals.cpp | ||
---|---|---|
2 | Update file comment. |
LGTM
(Don't forget to resolve the STLExtras diffbase first)
mlir/lib/IR/OperationSupport.cpp | ||
---|---|---|
552 | 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; | |
594 | Same as below. | |
596 | ? Or allBlockMatch.getValueOr(false) | |
640–655 | Would a lambda that removed the duplication here be cleaner? | |
mlir/lib/Transforms/CSE.cpp | ||
32–33 | 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? | |
44–53 | I feel like we should have a util for the default comparison/hash as well as the ignore case. | |
mlir/test/Transforms/cse.mlir | ||
267 | Unrelated? | |
mlir/test/lib/IR/TestOperationEquals.cpp | ||
29–30 |
mlir/lib/Transforms/CSE.cpp | ||
---|---|---|
32–33 | I need to wrap (unless you see another way) because: ERROR: mlir/lib/Transforms/CSE.cpp:33:26 no viable conversion from '<overloaded function type>' to 'function_ref<llvm::hash_code (mlir::Value)>' /*hashOperands=*/hash_value, ^~~~~~~~~~ llvm/include/llvm/ADT/STLExtras.h:171:7 candidate constructor (the implicit copy constructor) not viable: no overload of 'hash_value' matching 'const llvm::function_ref<llvm::hash_code (mlir::Value)> &' for 1st argument class function_ref<Ret(Params...)> { ^ llvm/include/llvm/ADT/STLExtras.h:171:7 candidate constructor (the implicit move constructor) not viable: no overload of 'hash_value' matching 'llvm::function_ref<llvm::hash_code (mlir::Value)> &&' for 1st argument llvm/include/llvm/ADT/STLExtras.h:183:3 candidate constructor not viable: no overload of 'hash_value' matching 'std::nullptr_t' (aka 'nullptr_t') for 1st argument function_ref(std::nullptr_t) {} ^ llvm/include/llvm/ADT/STLExtras.h:186:3 candidate template ignored: couldn't infer template argument 'Callable' function_ref( ^ mlir/include/mlir/IR/OperationSupport.h:915:44 passing argument to parameter 'hashOperands' here function_ref<llvm::hash_code(Value)> hashOperands = ^ |
Can you pull the functors out as separate variables to reduce indent here?