[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:

OK so now you describe a behavior change, but your PR title description didn't mention it: please be very mindful about not misrepresenting your changes. A behavior change isn't a "refactor".

Talking about behavior change, it should always come with a test.

Looking at your diff: how is your new logic correct though? You identified that the original test should be `if (!result.second) `, but you actually entirely removed it?  You write `if (!memEffectsCache.contains(fromOp)) {` but there is no `else` to guard the `Check if the cached operation has a MemoryEffects::Write` part?


Finally, your rewrite hits the cache map three times instead of one for the original code. The try_emplace and check on `.second` is an idiomatic pattern to follow.


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