xlauko wrote: > @xlauko @erichkeane — looked into both suggestions and pushed an update > ([20eaaaf](https://github.com/llvm/llvm-project/commit/20eaaaf780e86ab5ce15c0c1cec5835a6fd9a749)) > that adopts Erich's "have SOME sort of member" pattern: > `SymbolTableCollection` is now a member of `LoweringPreparePass`, and all the > threaded `mlir::SymbolTableCollection &` parameters are gone from this pass's > helper signatures. > > A few notes: > > * The change required a custom copy constructor. `SymbolTableCollection` > owns `unique_ptr<SymbolTable>` entries so the implicit copy constructor is > ill-formed, and MLIR's `clonePass()` requires copy construction. The cloned > pass starts with a fresh empty cache (and fresh empty per-run state more > generally), which matches MLIR convention for pass clones and is actually > more correct than the previous default-generated behavior that silently > copied per-run state. > * I did NOT switch to `mlir::SymbolTableAnalysis` (xlauko's other > suggestion). Two reasons: > > 1. `SymbolTableAnalysis` has zero existing users in the LLVM monorepo today > — we'd be adopters [Fixing Rust build > #1](https://github.com/llvm/llvm-project/pull/1). Probably worth doing > eventually, but as a pipeline-wide change rather than per-PR. > 2. The cross-pass cache only stays valid if every pass that mutates symbols > uses `SymbolTable::insert` / `rename` / `erase` (or calls > `invalidateSymbolTable`). CIR currently doesn't honor that anywhere — > `LoweringPrepare::buildRuntimeFunction`, `getOrCreateRuntimeVariable`, the > new-LLVM-func paths in `LowerToLLVM`, etc., all use raw > `OpBuilder::create<FuncOp>`. Adopting `SymbolTableAnalysis` without first > auditing and converting all those sites would silently produce stale cache > entries — that's the invalidation-error-proneness Erich was worried about. > > Within this pass, the member-on-pass version is correct by construction: > every cached `lookupNearestSymbolFrom` resolves the callee of a pre-existing > `cir.call`, never a freshly-created symbol. Documented as an invariant on the > member. > > For #195916 (the LowerToLLVM patterns): the `SymbolTableCollection` is > already stored as a member on the pattern struct (passed via the constructor > in `runOnOperation`). That's already Erich's "member" pattern; the only API > noise is one parameter on a free helper function (`rewriteCallOrInvoke`). I > think that one is fine as-is, but happy to refactor similarly if you'd prefer. > > @erichkeane @xlauko — does the new shape look right?
I agree with proposed approach that SymbolTableAnalysis should be adopted systematically, though you can technically in similar manner invalidate SymbolTableCollection cache without noticing in present solution. https://github.com/llvm/llvm-project/pull/195919 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
