ymandel accepted this revision. ymandel added a comment. This revision is now accepted and ready to land.
Much clearer now, thanks for the changes! The only real question I have is what's the best design for the cache -- lambda or explicit pair. I leave it up to you. In D122121#3396096 <https://reviews.llvm.org/D122121#3396096>, @sgatev wrote: > 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? Yes, it does! Thanks for clarifying. >> 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. Right. This division makes sense, particularly the point about responsibility. Point 2 is very much independent of `MatchSwitch`. But, I think matching the `optional` type is about both really in that every time we call the matcher we'll be checking the optional type, even if we only check it once *within* a single match. Still, fixing it doen't belong in `MatchSwitch`. > Do you think it's possible and makes sense to solve it in the AST matcher > framework? Would that address your concerns? Interesting idea -- basically, we'd need to build a caching mechanism into matchers based on names (identity really). That might make sense in general -- it's certainly a general concern -- but I'd be more inclined to try to fix it locally for now. That said, before we go there I'd want to profile the code, so no rush on this regardless. ================ Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:162 + static std::function<void(State &)> + BuildMatcherForStmt(const ast_matchers::internal::DynTypedMatcher &Matcher, + const std::vector<Action> &Actions, const Stmt &Stmt, ---------------- nit: maybe replace `Matcher` with `Action` or `SpecializedAction`? ================ Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:176-180 + return [&Actions, Index, &Stmt, Res = std::move(Results[0]), + &Context](State &S) { + Actions[Index]( + &Stmt, ast_matchers::MatchFinder::MatchResult(Res, &Context), S); + }; ---------------- Given that we only use `Actions` and `Index` in `Actions[Index]`, what about only stashing that? Similarly, for `MatchResult`? e.g. ``` [&Action = Actions[Index], Result = ast_matchers::MatchFinder::MatchResult(Results[0], &Context), &Stmt] ... ``` Alternatively, cache the pair of `Action, Result` directly instead of caching a lambda. This saves the path through a `std::function` and might make the code more readable (although, that's a matter of taste). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122121/new/ https://reviews.llvm.org/D122121 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
