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

[llvmlistbot at llvm.org]

================
@@ -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];
----------------
zackc6 wrote:

Thanks a lot for reviewing :)

Before this change 

197 - 209 
``` c++
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;
  }
}
```

It tried to add fromOp into the cache (try_emplace).  According to the API, if fromOp already exists in the the cache it will return false (results.second = false), otherwise insert (Key, Value) = (fromOp, std::make_pair(fromOp, nullptr) and return (results.second = true)

so line 199,  should changed to 
``` c++
if (!result.second) 
```
to align to what it intended to do.

Since this bug is a little hard to read at first glance, I tried to fix it and explicitly rewrite the steps, let the code document itself



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