[PATCH] D79134: [mlir] Add support for merging identical blocks during canonicalization

[Mehdi AMINI via Phabricator via llvm-commits]

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

The whole handling of `mismatchedOperands` caused me some mental effort to follow, I think it'd be nice to have a high-level description of this parts (which is more than "identical block" strictly speaking).



================
Comment at: mlir/include/mlir/IR/Operation.h:560
+  bool isUsedOutsideOfBlock(Block *block) {
+    return llvm::all_of(getOpResults(), [block](OpResult result) {
+      return result.isUsedOutsideOfBlock(block);
----------------
I'm surprised this isn't an "any_of" here, it can be surprising with the name of the method, and the difference with the same method on the Value class.


================
Comment at: mlir/include/mlir/IR/OperationSupport.h:856
+  static bool isEquivalentTo(Operation *lhs, Operation *rhs,
+                             unsigned flags = 0);
 };
----------------
What about LLVM_MARK_AS_BITMASK_ENUM and use the Enum instead of unsigned?


================
Comment at: mlir/lib/Transforms/Utils/RegionUtils.cpp:379
+/// This class contains the information for comparing the equivalencies of two
+/// blocks.
+struct BlockEquivalenceData {
----------------
can you add a description of what is considered "equivalent"?


================
Comment at: mlir/lib/Transforms/Utils/RegionUtils.cpp:392
+  /// A map of result producing operations to their relative orders within this
+  /// block.
+  DenseMap<Operation *, unsigned> opOrderIndex;
----------------
Can you expand here?
In particular the order here is counting the results of an operation: an operation order in a block is the number of defined value in the block before this operation (including the block arguments)


================
Comment at: mlir/lib/Transforms/Utils/RegionUtils.cpp:481
+    for (int operand : llvm::seq<int>(0, lhsIt->getNumOperands())) {
+      auto lhsOperand = lhsOperands[operand], rhsOperand = rhsOperands[operand];
+      if (lhsOperand == rhsOperand)
----------------
`auto`->`Value`?


================
Comment at: mlir/lib/Transforms/Utils/RegionUtils.cpp:485
+
+      // Let the operands differ if they are defined in a different block. These
+      // will become new arguments if the blocks get merged.
----------------
The comment seems misleading to me (or misplaced, I would write this before the line `if (!lhsIsInBlock) {`




================
Comment at: mlir/lib/Transforms/Utils/RegionUtils.cpp:508
+  if (lhsIt != lhsE || rhsIt != rhsE)
+    return failure();
+
----------------
Could check this ahead using the block ordering of the terminator?


================
Comment at: mlir/lib/Transforms/Utils/RegionUtils.cpp:519
+/// their operands updated.
+static bool unableToUpdatePredOperands(Block *block) {
+  for (auto it = block->pred_begin(), e = block->pred_end(); it != e; ++it) {
----------------
Nit: in general predicate a more readable when they are written with positive logic instead of negative ("able" instead of "unable")


================
Comment at: mlir/lib/Transforms/Utils/RegionUtils.cpp:539
+    // TODO: We could try and sub-partition this cluster if only some blocks
+    // cause the mismatch.
+    if (unableToUpdatePredOperands(leaderBlock) ||
----------------
You could get this by checking this property in `addToCluster` and return a failure when there are operandsToMerge so that you don't cluster blocks in the first place


================
Comment at: mlir/lib/Transforms/Utils/RegionUtils.cpp:578
+    }
+    // Update the predecessors for each of the blocks.
+    auto updatePredecessors = [&](Block *block, unsigned it) {
----------------
I didn't quite get what this meant until I read the code, what about something around the line of: `// Update each block's predecessors terminator with the new arguments` 


================
Comment at: mlir/lib/Transforms/Utils/RegionUtils.cpp:579
+    // Update the predecessors for each of the blocks.
+    auto updatePredecessors = [&](Block *block, unsigned it) {
+      for (auto i = block->pred_begin(), e = block->pred_end(); i != e; ++i) {
----------------
Do you have some name more explicit than `it`?


================
Comment at: mlir/lib/Transforms/Utils/RegionUtils.cpp:580
+    auto updatePredecessors = [&](Block *block, unsigned it) {
+      for (auto i = block->pred_begin(), e = block->pred_end(); i != e; ++i) {
+        auto branch = cast<BranchOpInterface>((*i)->getTerminator());
----------------
Can you use something else than `i` for the iterator? I'm already not thrilled by `i` for integer index, but here it seems misleading as it is an iterator that it derefenced.


================
Comment at: mlir/lib/Transforms/Utils/RegionUtils.cpp:623
+      // Don't allow merging if this block has any regions.
+      // TODO: Ådd support for regions if necessary.
+      bool hasNonEmptyRegion = llvm::any_of(*block, [](Operation &op) {
----------------
Å->A


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79134