[llvm-branch-commits] [mlir] [mlir][IR] Add listener notifications for pattern begin/end (PR #84131)

[Matthias Springer via llvm-branch-commits]

================
@@ -68,9 +68,9 @@ class PatternApplicator {
   ///            invalidate the match and try another pattern.
   LogicalResult
   matchAndRewrite(Operation *op, PatternRewriter &rewriter,
-                  function_ref<bool(const Pattern &)> canApply = {},
-                  function_ref<void(const Pattern &)> onFailure = {},
-                  function_ref<LogicalResult(const Pattern &)> onSuccess = {});
+                  std::function<bool(const Pattern &)> canApply = {},
+                  std::function<void(const Pattern &)> onFailure = {},
+                  std::function<LogicalResult(const Pattern &)> onSuccess = {});
----------------
matthias-springer wrote:

There is a `stack-use-after-scope` (reported by ASAN, also crashes in opt mode) with `function_ref`. The callback in the greedy pattern rewriter is defined inside of an `if` check:
```c++
    function_ref<bool(const Pattern &)> canApply = {};
    function_ref<void(const Pattern &)> onFailure = {};
    function_ref<LogicalResult(const Pattern &)> onSuccess = {};
    bool debugBuild = false;
#ifdef NDEBUG
    debugBuild = true;
#endif // NDEBUG
    if (debugBuild || config.listener) {
      canApply = [&](const Pattern &pattern) {
        if (this->config.listener) { /* ... */ }
        // ...
      }
      // ...
    }

    // `canApply` points to a lambda that is out of scope.
    LogicalResult matchResult =
        matcher.matchAndRewrite(op, *this, canApply, onFailure, onSuccess);
```

`function_ref` is a "non-owning function wrapper", but the lambda captures `this`.

Changing to `std::function` is one way to fix it. I could also just always pass a lambda. That would actually be my preferred solution, but there is a slight overhead when running in opt mode and without listener because the callback would always be called (even if it does not do anything):
```c++
    LogicalResult matchResult = matcher.matchAndRewrite(
        op, *this,
        /*canApply=*/[&](const Pattern &pattern) {
          if (this->listener) { /* ... */ }
          // ...
        },
        /*onFailure=*/[&](const Pattern &pattern) { /* ... */},
        /*onSuccess=*/[&](const Pattern &pattern) { /* ... */});
```

What do you think?


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