Skip to content

[mlir][CSE] Introduce hoist-pure-ops logic to CSE pass#197397

Closed
linuxlonelyeagle wants to merge 1 commit into
llvm:mainfrom
linuxlonelyeagle:update-cse-2
Closed

[mlir][CSE] Introduce hoist-pure-ops logic to CSE pass#197397
linuxlonelyeagle wants to merge 1 commit into
llvm:mainfrom
linuxlonelyeagle:update-cse-2

Conversation

@linuxlonelyeagle
Copy link
Copy Markdown
Member

This PR is based on this theory: if an Op is a Pure Op, we have the opportunity to hoist its position based on SSA dominance. This logic has now been incorporated into the CSE pass, now we can use it to further optimize the IR to achieve more concise code. The original PR: #180556.
RFC: https://discourse.llvm.org/t/rfc-mlir-introduce-hoist-pure-ops-pass/88903

@llvmorg-github-actions
Copy link
Copy Markdown

llvmorg-github-actions Bot commented May 13, 2026

@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-mlir-bufferization
@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir-linalg

Author: lonely eagle (linuxlonelyeagle)

Changes

This PR is based on this theory: if an Op is a Pure Op, we have the opportunity to hoist its position based on SSA dominance. This logic has now been incorporated into the CSE pass, now we can use it to further optimize the IR to achieve more concise code. The original PR: #180556.
RFC: https://discourse.llvm.org/t/rfc-mlir-introduce-hoist-pure-ops-pass/88903


Patch is 83.10 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/197397.diff

17 Files Affected:

  • (modified) flang/lib/Optimizer/Passes/Pipelines.cpp (+3-1)
  • (modified) mlir/include/mlir/Transforms/CSE.h (+4-2)
  • (modified) mlir/include/mlir/Transforms/Passes.td (+4)
  • (modified) mlir/lib/Transforms/CSE.cpp (+4-1)
  • (modified) mlir/lib/Transforms/Utils/CSE.cpp (+213-25)
  • (modified) mlir/test/Conversion/ArmSMEToLLVM/tile-spills-and-fills.mlir (+4-8)
  • (modified) mlir/test/Dialect/Bufferization/Transforms/one-shot-bufferize-empty-tensor-elimination.mlir (+4-11)
  • (modified) mlir/test/Dialect/Linalg/matmul-shared-memory-padding.mlir (+1-2)
  • (modified) mlir/test/Dialect/Linalg/transform-op-pad.mlir (+1-1)
  • (modified) mlir/test/Dialect/SparseTensor/sparse_fill_zero.mlir (+105-106)
  • (modified) mlir/test/Dialect/SparseTensor/sparse_kernels_to_iterator.mlir (+57-61)
  • (modified) mlir/test/Dialect/SparseTensor/sparse_vector_index.mlir (+42-43)
  • (modified) mlir/test/Pass/ir-printing.mlir (+12-12)
  • (modified) mlir/test/Pass/run-reproducer.mlir (+4-4)
  • (modified) mlir/test/Transforms/composite-pass.mlir (+1-1)
  • (modified) mlir/test/Transforms/cse.mlir (+127-17)
  • (modified) mlir/test/python/pass_manager.py (+2-2)
diff --git a/flang/lib/Optimizer/Passes/Pipelines.cpp b/flang/lib/Optimizer/Passes/Pipelines.cpp
index 920d6f86a355e..80a0b58b6ab63 100644
--- a/flang/lib/Optimizer/Passes/Pipelines.cpp
+++ b/flang/lib/Optimizer/Passes/Pipelines.cpp
@@ -293,7 +293,9 @@ void createHLFIRToFIRPassPipeline(mlir::PassManager &pm,
       pm, hlfir::createInlineElementals);
   if (optLevel.isOptimizingForSpeed()) {
     addCanonicalizerPassWithoutRegionSimplification(pm);
-    pm.addPass(mlir::createCSEPass());
+    mlir::CSEPassOptions options;
+    options.hoistPureOps = false;
+    pm.addPass(mlir::createCSEPass(options));
     // Run SimplifyHLFIRIntrinsics pass late after CSE,
     // and allow introducing operations with new side effects.
     addNestedPassToAllTopLevelOperations(pm, [&]() {
diff --git a/mlir/include/mlir/Transforms/CSE.h b/mlir/include/mlir/Transforms/CSE.h
index 4a87d585e0eb9..b930b78cb641f 100644
--- a/mlir/include/mlir/Transforms/CSE.h
+++ b/mlir/include/mlir/Transforms/CSE.h
@@ -32,7 +32,8 @@ void eliminateCommonSubExpressions(RewriterBase &rewriter,
                                    DominanceInfo &domInfo, Operation *op,
                                    bool *changed = nullptr,
                                    int64_t *numCSE = nullptr,
-                                   int64_t *numDCE = nullptr);
+                                   int64_t *numDCE = nullptr,
+                                   bool hoistPureOps = true);
 
 /// Eliminate common subexpressions within the given region.
 ///
@@ -41,7 +42,8 @@ void eliminateCommonSubExpressions(RewriterBase &rewriter,
 /// DCE counts are needed.
 void eliminateCommonSubExpressions(RewriterBase &rewriter,
                                    DominanceInfo &domInfo, Region &region,
-                                   bool *changed = nullptr);
+                                   bool *changed = nullptr,
+                                   bool hoistPureOps = true);
 
 } // namespace mlir
 
diff --git a/mlir/include/mlir/Transforms/Passes.td b/mlir/include/mlir/Transforms/Passes.td
index 74ac370ea950b..e7781c8c9e1bb 100644
--- a/mlir/include/mlir/Transforms/Passes.td
+++ b/mlir/include/mlir/Transforms/Passes.td
@@ -94,6 +94,10 @@ def CSEPass : Pass<"cse"> {
     operations. See [Common subexpression elimination](https://en.wikipedia.org/wiki/Common_subexpression_elimination)
     for more general details on this optimization.
   }];
+  let options = [
+    Option<"hoistPureOps", "hoist-pure-ops", "bool", /*default=*/"true",
+           "Allow hoisting of pure operations out of regions">,
+  ];
   let statistics = [
     Statistic<"numCSE", "num-cse'd", "Number of operations CSE'd">,
     Statistic<"numDCE", "num-dce'd", "Number of operations DCE'd">
diff --git a/mlir/lib/Transforms/CSE.cpp b/mlir/lib/Transforms/CSE.cpp
index f7afa03e2f02b..4ed90d49acea7 100644
--- a/mlir/lib/Transforms/CSE.cpp
+++ b/mlir/lib/Transforms/CSE.cpp
@@ -13,6 +13,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "mlir/Transforms/CSE.h"
+#include "mlir/Transforms/Passes.h"
 
 #include "mlir/IR/Dominance.h"
 #include "mlir/IR/PatternMatch.h"
@@ -23,11 +24,13 @@ namespace mlir {
 #include "mlir/Transforms/Passes.h.inc"
 } // namespace mlir
 
+#define DEBUG_TYPE "cse"
 using namespace mlir;
 
 namespace {
 /// CSE pass.
 struct CSE : public impl::CSEPassBase<CSE> {
+  using impl::CSEPassBase<CSE>::CSEPassBase;
   void runOnOperation() override;
 };
 } // namespace
@@ -41,7 +44,7 @@ void CSE::runOnOperation() {
   int64_t cseCount = 0;
   int64_t dceCount = 0;
   eliminateCommonSubExpressions(rewriter, domInfo, getOperation(), &changed,
-                                &cseCount, &dceCount);
+                                &cseCount, &dceCount, hoistPureOps);
 
   numCSE = cseCount;
   numDCE = dceCount;
diff --git a/mlir/lib/Transforms/Utils/CSE.cpp b/mlir/lib/Transforms/Utils/CSE.cpp
index 90444e6201891..934908bda7883 100644
--- a/mlir/lib/Transforms/Utils/CSE.cpp
+++ b/mlir/lib/Transforms/Utils/CSE.cpp
@@ -19,6 +19,7 @@
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/ScopedHashTable.h"
 #include "llvm/Support/Allocator.h"
+#include "llvm/Support/DebugLog.h"
 #include "llvm/Support/RecyclingAllocator.h"
 #include <deque>
 
@@ -52,8 +53,9 @@ namespace {
 /// Simple common sub-expression elimination.
 class CSEDriver {
 public:
-  CSEDriver(RewriterBase &rewriter, DominanceInfo *domInfo)
-      : rewriter(rewriter), domInfo(domInfo) {}
+  CSEDriver(RewriterBase &rewriter, DominanceInfo *domInfo,
+            bool hoistPureOps = true)
+      : rewriter(rewriter), domInfo(domInfo), hoistPureOps(hoistPureOps) {}
 
   /// Simplify all operations within the given op.
   void simplify(Operation *op, bool *changed = nullptr);
@@ -97,10 +99,13 @@ class CSEDriver {
 
   /// Attempt to eliminate a redundant operation. Returns success if the
   /// operation was marked for removal, failure otherwise.
-  LogicalResult simplifyOperation(ScopedMapTy &knownValues, Operation *op,
+  LogicalResult simplifyOperation(ScopedMapTy &knownValues,
+                                  ScopedMapTy &knownPureOps, Operation *op,
                                   bool hasSSADominance);
-  void simplifyBlock(ScopedMapTy &knownValues, Block *bb, bool hasSSADominance);
-  void simplifyRegion(ScopedMapTy &knownValues, Region &region);
+  void simplifyBlock(ScopedMapTy &knownValues, ScopedMapTy &knownPureOps,
+                     Block *bb, bool hasSSADominance);
+  void simplifyRegion(ScopedMapTy &knownValues, ScopedMapTy &knownPureOps,
+                      Region &region);
 
   /// Erase all operations queued for deletion by the simplification routines.
   void eraseDeadOps(bool *changed);
@@ -112,20 +117,137 @@ class CSEDriver {
   /// between the two operations.
   bool hasOtherSideEffectingOpInBetween(Operation *fromOp, Operation *toOp);
 
+  LogicalResult hoistPureOp(Operation *existing, Operation *op);
+
   /// A rewriter for modifying the IR.
   RewriterBase &rewriter;
 
   /// Operations marked as dead and to be erased.
   std::vector<Operation *> opsToErase;
+
   DominanceInfo *domInfo = nullptr;
   MemEffectsCache memEffectsCache;
 
   // Various statistics.
   int64_t numCSE = 0;
   int64_t numDCE = 0;
+
+  bool hoistPureOps = true;
+
+  // The map uses region op as the key and a list of operations as the
+  // value. This list describes the dependencies of the region op, as the
+  // operations within the region op consume results from the value in
+  // the map. Therefore, it is necessary to consider these dependent operations
+  // when hoisting a region op.
+  DenseMap<Operation *, SmallVector<Operation *>> hoistBlockingDeps;
+
+  // The keys of this map are exiting ops, and the values are lists of
+  // operations. Each entry describes an exiting op that has been hoisted along
+  // with its associated operations. When an exiting op and its equivalent
+  // operations are hoisted for the first time, they are added to this map.
+  // During subsequent hoistings of the same exiting op(hoisting exiting op
+  // multiple times), the operations stored in the map's value will also be
+  // hoisted together with it.
+  DenseMap<Operation *, SmallVector<Operation *>> hoistOpsSet;
 };
 } // namespace
 
+/// Returns true if the path between block 'a' and block 'b' in the region
+/// hierarchy crosses an operation with the 'IsIsolatedFromAbove' trait.
+static bool isBlockCrossIsIsolatedFromAbove(DominanceInfo *dominate, Block *a,
+                                            Block *b) {
+  if (a == b)
+    return false;
+  if (a->getParent() == b->getParent())
+    return false;
+  if (dominate->dominates(b, a))
+    std::swap(b, a);
+  while (b && b->getParentOp()) {
+    Operation *parentOp = b->getParentOp();
+    if (parentOp->mightHaveTrait<OpTrait::IsIsolatedFromAbove>())
+      return true;
+    b = parentOp->getBlock();
+    if (b == a)
+      return false;
+  }
+  return false;
+}
+
+/// Hoist the pure ops to the location of the Nearest Common Dominator.
+LogicalResult CSEDriver::hoistPureOp(Operation *existing, Operation *op) {
+  Block *ancestorBlock =
+      domInfo->findNearestCommonDominator(existing->getBlock(), op->getBlock());
+  if (!ancestorBlock) {
+    LDBG() << "hoist " << OpWithFlags(existing, OpPrintingFlags().skipRegions())
+           << " and " << OpWithFlags(op, OpPrintingFlags().skipRegions())
+           << " failed";
+    return failure();
+  }
+
+  if (isBlockCrossIsIsolatedFromAbove(domInfo, ancestorBlock,
+                                      existing->getBlock()) ||
+      isBlockCrossIsIsolatedFromAbove(domInfo, ancestorBlock, op->getBlock()))
+    return failure();
+
+  if (existing->getParentOp() != ancestorBlock->getParentOp() &&
+      !existing->use_empty()) {
+    LDBG() << "add "
+           << OpWithFlags(existing->getParentOp(),
+                          OpPrintingFlags().skipRegions())
+           << " dependents "
+           << OpWithFlags(existing, OpPrintingFlags().skipRegions());
+    hoistBlockingDeps[existing->getParentOp()].push_back(existing);
+  }
+
+  // Find the insertion point based on dominance relationships. When hoisting a
+  // region op, we must consider not only its operands but also the dominance
+  // relationships of the operations within the region when determining the
+  // insertion point
+  Operation *insertPoint = nullptr;
+  SmallVector<Value> dependentOperands(existing->getOperands());
+  if (hoistBlockingDeps.contains(existing) &&
+      !hoistBlockingDeps[existing].empty()) {
+    for (Operation *dependentOp : hoistBlockingDeps[existing])
+      dependentOperands.append(dependentOp->getResults().begin(),
+                               dependentOp->getResults().end());
+  }
+
+  for (Value operand : dependentOperands) {
+    if (domInfo->properlyDominates(operand, &ancestorBlock->front()))
+      continue;
+    if (!insertPoint) {
+      insertPoint = operand.getDefiningOp();
+    } else {
+      insertPoint = domInfo->dominates(insertPoint, operand.getDefiningOp())
+                        ? operand.getDefiningOp()
+                        : insertPoint;
+    }
+  }
+
+  // We hoist both `op` and `existing` here because if they are identical
+  // regionOps and we only hoist existing, the two would no longer be congruent.
+  // This would lead to a missed optimization opportunity in subsequent CSE
+  // passes. The test @cse_multiple_regions's `%r2` tests it.
+  if (!insertPoint) {
+    rewriter.moveOpBefore(existing, ancestorBlock, ancestorBlock->begin());
+    rewriter.moveOpAfter(op, existing);
+  } else {
+    rewriter.moveOpAfter(existing, insertPoint);
+    rewriter.moveOpAfter(op, existing);
+  }
+
+  // When hoisting an exiting op multiple times, we must also hoist the
+  // operations that were previously hoisted alongside it.
+  if (hoistOpsSet.contains(existing) && !hoistOpsSet[existing].empty())
+    for (Operation *op : hoistOpsSet[existing])
+      rewriter.moveOpAfter(op, existing);
+  hoistOpsSet[existing].push_back(op);
+  LDBG() << "hoist " << OpWithFlags(existing, OpPrintingFlags().skipRegions())
+         << " and " << OpWithFlags(op, OpPrintingFlags().skipRegions())
+         << " success";
+  return success();
+}
+
 void CSEDriver::replaceUsesAndDelete(ScopedMapTy &knownValues, Operation *op,
                                      Operation *existing,
                                      bool hasSSADominance) {
@@ -136,6 +258,21 @@ void CSEDriver::replaceUsesAndDelete(ScopedMapTy &knownValues, Operation *op,
     // If the region has SSA dominance, then we are guaranteed to have not
     // visited any use of the current operation.
     // Replace all uses, but do not remove the operation yet.
+    if (!domInfo->properlyDominates(existing, op)) {
+      if (!hoistPureOps || failed(hoistPureOp(existing, op)))
+        return;
+    } else {
+      // Hoist `op` even though `existing` already dominates it, because
+      // hoisting op may create further CSE optimization opportunities for
+      // subsequent region operations. The test @cse_multiple_regions's `%r3`
+      // tests it.
+      rewriter.moveOpAfter(op, existing);
+    }
+    LDBG() << "replace " << OpWithFlags(op, OpPrintingFlags().skipRegions())
+           << " with "
+           << OpWithFlags(existing, OpPrintingFlags().skipRegions());
+    LDBG() << "add " << OpWithFlags(op, OpPrintingFlags().skipRegions())
+           << " to opsToErase";
     rewriter.replaceAllOpUsesWith(op, existing->getResults());
     opsToErase.push_back(op);
   } else {
@@ -150,11 +287,19 @@ void CSEDriver::replaceUsesAndDelete(ScopedMapTy &knownValues, Operation *op,
         if (all_of(v.getUses(), wasVisited))
           rewriteListener->notifyOperationReplaced(op, existing);
 
+    if (!domInfo->properlyDominates(existing, op)) {
+      if (!hoistPureOps || failed(hoistPureOp(existing, op)))
+        return;
+    }
+    // Replace all uses, but do not remove the operation yet. This does not
+    // notify the listener because the original op is not erased.
+    LDBG() << "replace " << OpWithFlags(op, OpPrintingFlags().skipRegions())
+           << " with "
+           << OpWithFlags(existing, OpPrintingFlags().skipRegions());
     // Replace all uses, but do not remove the operation yet. This does not
     // notify the listener because the original op is not erased.
     rewriter.replaceUsesWithIf(op->getResults(), existing->getResults(),
                                wasVisited);
-
     // There may be some remaining uses of the operation.
     if (op->use_empty())
       opsToErase.push_back(op);
@@ -247,8 +392,11 @@ bool CSEDriver::hasOtherSideEffectingOpInBetween(Operation *fromOp,
 
 /// Attempt to eliminate a redundant operation.
 LogicalResult CSEDriver::simplifyOperation(ScopedMapTy &knownValues,
+                                           ScopedMapTy &knownPureOps,
                                            Operation *op,
                                            bool hasSSADominance) {
+  LDBG() << "visit operation: "
+         << OpWithFlags(op, OpPrintingFlags().skipRegions());
   // Don't simplify terminator operations.
   if (op->hasTrait<OpTrait::IsTerminator>())
     return failure();
@@ -279,6 +427,8 @@ LogicalResult CSEDriver::simplifyOperation(ScopedMapTy &knownValues,
         return success();
       }
     }
+    LDBG() << "insert op: " << OpWithFlags(op, OpPrintingFlags().skipRegions())
+           << " to map";
     knownValues.insert(op, op);
     return failure();
   }
@@ -289,13 +439,29 @@ LogicalResult CSEDriver::simplifyOperation(ScopedMapTy &knownValues,
     return success();
   }
 
-  // Otherwise, we add this operation to the known values map.
-  knownValues.insert(op, op);
+  if (auto *existing = knownPureOps.lookup(op)) {
+    replaceUsesAndDelete(knownPureOps, op, existing, hasSSADominance);
+    return success();
+  }
+
+  if (mlir::isPure(op)) {
+    LDBG() << "insert op: " << OpWithFlags(op, OpPrintingFlags().skipRegions())
+           << " to pureMap";
+    knownPureOps.insert(op, op);
+  } else {
+    // Otherwise, we add this operation to the known values map.
+    LDBG() << "insert op: " << OpWithFlags(op, OpPrintingFlags().skipRegions())
+           << " to map";
+    knownValues.insert(op, op);
+  }
   return failure();
 }
 
-void CSEDriver::simplifyBlock(ScopedMapTy &knownValues, Block *bb,
+void CSEDriver::simplifyBlock(ScopedMapTy &knownValues,
+                              ScopedMapTy &knownPureOps, Block *bb,
                               bool hasSSADominance) {
+  LDBG() << "visit block #" << bb->computeBlockNumber() << " of "
+         << OpWithFlags(bb->getParentOp(), OpPrintingFlags().skipRegions());
   for (auto &op : llvm::make_early_inc_range(*bb)) {
     // If the operation is already trivially dead just add it to the erase list.
     // This also avoids calling `simplifyRegion` on dead region ops
@@ -313,34 +479,42 @@ void CSEDriver::simplifyBlock(ScopedMapTy &knownValues, Block *bb,
       // implicit captures in explicit capture only regions.
       if (op.mightHaveTrait<OpTrait::IsIsolatedFromAbove>()) {
         ScopedMapTy nestedKnownValues;
+        ScopedMapTy nestedKnownPureOps;
+        ScopedMapTy::ScopeTy scope(nestedKnownValues);
+        ScopedMapTy::ScopeTy pureScope(nestedKnownPureOps);
         for (auto &region : op.getRegions())
-          simplifyRegion(nestedKnownValues, region);
+          simplifyRegion(nestedKnownValues, nestedKnownPureOps, region);
       } else {
         // Otherwise, process nested regions normally.
         for (auto &region : op.getRegions())
-          simplifyRegion(knownValues, region);
+          simplifyRegion(knownValues, knownPureOps, region);
       }
     }
 
     // If the operation is simplified, we don't process any held regions.
-    if (succeeded(simplifyOperation(knownValues, &op, hasSSADominance)))
+    if (succeeded(
+            simplifyOperation(knownValues, knownPureOps, &op, hasSSADominance)))
       continue;
   }
   // Clear the MemoryEffects cache since its usage is by block only.
   memEffectsCache.clear();
 }
 
-void CSEDriver::simplifyRegion(ScopedMapTy &knownValues, Region &region) {
+void CSEDriver::simplifyRegion(ScopedMapTy &knownValues,
+                               ScopedMapTy &knownPureOps, Region &region) {
   // If the region is empty there is nothing to do.
   if (region.empty())
     return;
 
+  LDBG() << "visit region #" << region.getRegionNumber() << " of "
+         << OpWithFlags(region.getParentOp(), OpPrintingFlags().skipRegions());
+
   bool hasSSADominance = domInfo->hasSSADominance(&region);
 
   // If the region only contains one block, then simplify it directly.
   if (region.hasOneBlock()) {
     ScopedMapTy::ScopeTy scope(knownValues);
-    simplifyBlock(knownValues, &region.front(), hasSSADominance);
+    simplifyBlock(knownValues, knownPureOps, &region.front(), hasSSADominance);
     return;
   }
 
@@ -368,7 +542,7 @@ void CSEDriver::simplifyRegion(ScopedMapTy &knownValues, Region &region) {
     // Check to see if we need to process this node.
     if (!currentNode->processed) {
       currentNode->processed = true;
-      simplifyBlock(knownValues, currentNode->node->getBlock(),
+      simplifyBlock(knownValues, knownPureOps, currentNode->node->getBlock(),
                     hasSSADominance);
     }
 
@@ -402,24 +576,38 @@ void CSEDriver::eraseDeadOps(bool *changed) {
 }
 
 void CSEDriver::simplify(Operation *op, bool *changed) {
-  // Simplify all regions.
-  ScopedMapTy knownValues;
-  for (auto &region : op->getRegions())
-    simplifyRegion(knownValues, region);
+  /// Simplify all regions. Added a new scope using curly braces to release the
+  /// knownPureOps scope before deleting the operation.
+  {
+    /// The entry point for CSE simplification. A top-level scope is added for
+    /// 'knownPureOps' to track pure operations across the entire operation's
+    /// regions, enabling potential hoisting opportunities. Since only pure
+    /// operations are candidates for hoisting, 'knownValues' does not require
+    /// a corresponding top-level scope here.
+    ScopedMapTy knownValues;
+    ScopedMapTy knownPureOps;
+    ScopedMapTy::ScopeTy scope(knownPureOps);
+    for (auto &region : op->getRegions())
+      simplifyRegion(knownValues, knownPureOps, region);
+  }
   eraseDeadOps(changed);
 }
 
 void CSEDriver::simplify(Region &region, bool *changed) {
-  ScopedMapTy knownValues;
-  simplifyRegion(knownValues, region);
+  {
+    ScopedMapTy knownValues;
+    ScopedMapTy knownPureOps;
+    ScopedMapTy::ScopeTy scope(knownPureOps); 
+    simplifyRegion(knownValues, knownPureOps, region);
+  }
   eraseDeadOps(changed);
 }
 
 void mlir::eliminateCommonSubExpressions(RewriterBase &rewriter,
                                          DominanceInfo &domInfo, Operation *op,
                                          bool *changed, int64_t *numCSE,
-                                         int64_t *numDCE) {
-  CSEDriver driver(rewriter, &domInfo);
+                                         int64_t *numDCE, bool hoistPureOps) {
+  CSEDriver driver(rewriter, &domInfo, hoistPureOps);
   driver.simplify(op, changed);
   if (numCSE)
     *numCSE = driver.getNumCSE();
@@ -429,7 +617,7 @@ void mlir::eliminateCommonSubExpressions(RewriterBase &rewriter,
 
 void mlir::eliminateCommonSubExpressions(RewriterBase &rewriter,
 ...
[truncated]

@github-actions
Copy link
Copy Markdown

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff origin/main HEAD --extensions h,cpp -- flang/lib/Optimizer/Passes/Pipelines.cpp mlir/include/mlir/Transforms/CSE.h mlir/lib/Transforms/CSE.cpp mlir/lib/Transforms/Utils/CSE.cpp --diff_from_common_commit

⚠️
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing origin/main to the base branch/commit you want to compare against.
⚠️

View the diff from clang-format here.
diff --git a/mlir/lib/Transforms/Utils/CSE.cpp b/mlir/lib/Transforms/Utils/CSE.cpp
index 934908bda..fae226785 100644
--- a/mlir/lib/Transforms/Utils/CSE.cpp
+++ b/mlir/lib/Transforms/Utils/CSE.cpp
@@ -597,7 +597,7 @@ void CSEDriver::simplify(Region &region, bool *changed) {
   {
     ScopedMapTy knownValues;
     ScopedMapTy knownPureOps;
-    ScopedMapTy::ScopeTy scope(knownPureOps); 
+    ScopedMapTy::ScopeTy scope(knownPureOps);
     simplifyRegion(knownValues, knownPureOps, region);
   }
   eraseDeadOps(changed);

@joker-eph
Copy link
Copy Markdown
Contributor

Why a new PR? What changed since #180556 ?

@linuxlonelyeagle
Copy link
Copy Markdown
Member Author

Why a new PR? What changed since #180556 ?

The previous PR had a lot of commits, and rebasing each one individually was becoming difficult. While I could have squashed them, I didn't want to lose the intermediate history. To preserve the records while keeping things clean, I created a new branch, squashed the old commits into a single base, and then performed the rebase

The current implementation is a bit complex. I'll review it and submit a new commit to simplify it later.

@joker-eph
Copy link
Copy Markdown
Contributor

The previous PR had a lot of commits, and rebasing each one individually was becoming difficult.

Why didn't you merge main into the branch instead?

squashed the old commits into a single base, and then performed the rebase

This is why I always squash and rebase, but I'm missing the "open a new PR" part: we are losing review history and it wouldn't be sustainable to re-open a new PR every time you need a rebase.

@linuxlonelyeagle
Copy link
Copy Markdown
Member Author

The previous PR had a lot of commits, and rebasing each one individually was becoming difficult.

Why didn't you merge main into the branch instead?

squashed the old commits into a single base, and then performed the rebase

This is why I always squash and rebase, but I'm missing the "open a new PR" part: we are losing review history and it wouldn't be sustainable to re-open a new PR every time you need a rebase.

Merge main is good. I have do it. 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flang:fir-hlfir flang Flang issues not falling into any other category mlir:bufferization Bufferization infrastructure mlir:core MLIR Core Infrastructure mlir:linalg mlir:sparse mlir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants