[clang] [CIR] Use SymbolTableCollection in LoweringPrepare's getCalledFunction (PR #195919)

Henrich Lauko via cfe-commits cfe-commits at lists.llvm.org
Sun May 10 06:23:25 PDT 2026


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


More information about the cfe-commits mailing list