[Mlir-commits] [mlir] [mlir][Transforms] Refactor CSE side-effect cache access in read-conflict scan (PR #192178)

[Mehdi Amini]

================
@@ -194,29 +194,32 @@ bool CSEDriver::hasOtherSideEffectingOpInBetween(Operation *fromOp,
   }
 
   Operation *nextOp = fromOp->getNextNode();
-  auto result =
-      memEffectsCache.try_emplace(fromOp, std::make_pair(fromOp, nullptr));
-  if (result.second) {
-    auto memEffectsCachePair = result.first->second;
-    if (memEffectsCachePair.second == nullptr) {
-      // No MemoryEffects::Write has been detected until the cached operation.
-      // Continue looking from the cached operation to toOp.
-      nextOp = memEffectsCachePair.first;
-    } else {
-      // MemoryEffects::Write has been detected before so there is no need to
-      // check further.
-      return true;
-    }
+
+  if (!memEffectsCache.contains(fromOp)) {
+    memEffectsCache.insert(
+        std::make_pair(fromOp, std::make_pair(fromOp, nullptr)));
   }
+
+  auto &memEffectsCachePair = memEffectsCache[fromOp];
----------------
joker-eph wrote:

> now you describe a behavior change

I looked into it further, the bug is performance-only, not a correctness bug, hence can't be tested (the PR should still document this faithfully).

It works because right now we're just skipping the cache and always doing the recomputation.

Your lack of else is just an inefficiency of looking up the cache entry we just inserted, which will always be a no-op.

The best fix remains just inverting the original condition I think.


https://github.com/llvm/llvm-project/pull/192178