[PATCH] D122121: [clang][dataflow] Add action caching support to MatchSwitch

Mon Mar 21 05:56:05 PDT 2022

sgatev marked an inline comment as done.
sgatev added a comment.

In D122121#3396022 <https://reviews.llvm.org/D122121#3396022>, @ymandel wrote:

> What is the motivation for stashing the results of a match on a statement? Do we expect to encounter the same statement often?

The matcher is evaluated for each fixpoint iteration so I expect that we'll encounter the same statement as many times as there are iterations over the same basic block. Does that make sense?

> I thought the concern was more specific to re-matching particular types, like `std::optional`. For that, we could lazily store the declaration nodes (like we do in https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/Models/ChromiumCheckModel.cpp with `CheckDecls`).  But, I'm less clear on the impact of storing the result of the match itself.

I was thinking that maybe we can split the problem in two:

1. duplicated work across `matchDynamic` calls, and
2. duplicated work within a single `matchDynamic` call.

This patch addresses 1. as I see it as responsibility of `MatchSwitch`. Your comment seems to be about 2., if I'm reading it correctly. Do you think it's possible and makes sense to solve it in the AST matcher framework? Would that address your concerns?

================
Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:159
+  static std::function<void(State &)>
+  BuildMatcherForStmt(const ast_matchers::internal::DynTypedMatcher &Matcher,
+                      const std::vector<Action> &Actions, const Stmt &Stmt,
----------------
ymandel wrote:
> What is this function doing?  Please add a comment. Also, I don't understand how it compiles -- the return type is `std::function<void(State &)>` but it returns a lambda of type `(State &S) -> std::function<void(State &)>`?
The outer lambda was a leftover. I removed it and added a comment.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122121/new/

https://reviews.llvm.org/D122121