ltoplugin: preserve names for shared sret slots - #2345
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix LTO plugin shared sret refinement
Solid, well-targeted fix. The core correctness invariant holds: when multiple MethodByName calls share one sret slot, the pass now groups them (via ProcessedCalls + shared-storage matching) and refines the shared checked-loads with the union of every provably-known name, rather than letting the first call claim all of them. The all-or-nothing KnownNames fallback is preserved — if any grouped call has an unprovable name, all generic markers are left intact so GlobalDCE stays conservative. Memory safety, iterator validity across the IR edits, and recursion-depth bounds all check out. The @shared_sret / @shared_sret_unknown FileCheck cases exercise both the refine and the conservative-fallback paths.
A few non-blocking notes are inline. None are correctness bugs.
Suggestions (no change required):
- Consider a test case where two grouped calls request the same constant name, with a
CHECK-COUNT-1assertion, to lock in theaddNamededup invariant that the union relies on (the current test uses three distinct names).
| Value *SRetStorage = getSRetStorage(ReflectCall); | ||
| SmallVector<CallBase *, 4> SharedCalls{ReflectCall}; | ||
| if (SRetStorage) { | ||
| for (CallBase *Candidate : Calls) { |
There was a problem hiding this comment.
Minor (perf): this inner scan re-walks the entire Calls vector for every unprocessed ReflectCall, and getSRetStorage(Candidate) re-runs stripPointerCasts() each time — so the grouping is O(n^2) over the number of MethodByName call sites, with repeated pointer-cast walks. Correctness is fine (the ProcessedCalls set prevents reprocessing), and n is small in practice since Calls only holds attribute-tagged reflect calls. If you want to harden against large modules, a single pre-pass building DenseMap<Value* /*sret storage*/, SmallVector<CallBase*>> collapses this to O(n) and computes each storage once. Not required.
There was a problem hiding this comment.
Agreed on the complexity tradeoff. I kept the small scan because this vector only contains attribute-tagged reflect calls and is expected to stay small; a DenseMap grouping pass would be a reasonable follow-up if large-module profiles show this matters.
There was a problem hiding this comment.
Follow-up: I implemented the suggested pre-grouping with DenseMap after all. Same-machine full plugin coverage was 198.6s with pre-grouping versus about 200.3s before, so the long CI success path was mostly later demos rather than a measured regression, but this removes the O(n^2) scan and computes each cast-stripped storage once.
| bool KnownNames = collectStringSetFromCallArgs(ReflectCall, DL, Names); | ||
| bool KnownNames = true; | ||
| for (CallBase *SharedCall : SharedCalls) { | ||
| if (!collectStringSetFromCallArgs(SharedCall, DL, Names)) { |
There was a problem hiding this comment.
Behavior note: MaxReflectMethodNames (32) is now applied to the union of names across the whole shared group, not per call. A shared slot with several calls whose combined unique names exceed 32 will hit the cap in addName, set KnownNames=false, and refine none of them (falling back to the generic marker). This is safe/conservative, but the shared budget is a subtle change from the previous per-call limit — worth a one-line comment here so a future maintainer isn't surprised.
There was a problem hiding this comment.
Added a comment that the bounded name budget applies to the combined shared group and that exceeding it preserves the generic markers.
| continue; | ||
| } | ||
|
|
||
| // Optimizations can unroll several MethodByName calls while reusing one |
There was a problem hiding this comment.
Doc nit: "Optimizations can unroll several MethodByName calls" attributes the shared slot specifically to loop unrolling, but the code actually groups any distinct calls whose getSRetStorage result is pointer-identical (slot reuse / stack coloring / etc.) — the test itself uses three independent source-level calls, not an unrolled loop. Consider softening to something like "Optimizations can leave several MethodByName calls sharing one sret slot". Also note the "sees the loads for all of those calls" guarantee relies on the exact getSRetStorage(Candidate) == SRetStorage equality at line 1329 (cast-stripped identity, not aliasing) — fine today, just an implicit invariant.
There was a problem hiding this comment.
Updated the wording to cover any optimization that leaves calls sharing pointer-identical, cast-stripped sret storage rather than attributing it specifically to loop unrolling.
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
75070fb to
f908b33
Compare
|
Addressed the non-blocking review suggestions in |
f908b33 to
ec26599
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Summary
MethodByNamecalls that reuse the same sret storageMotivation
After PR #2344 changes bounds-check CFG shape, LLVM fully unrolls the range-literal fixture into three
MethodByNamecalls that reuse one sret alloca. The plugin previously walked every checked load reachable from that alloca while processing the first call, so all three loads were refined toQuery. GlobalDCE then incorrectly removedMutationandSubscription.This change keeps the refinement fail-closed while allowing known shared calls to retain only the union of their reachable method names.
Validation
Querycltest selection passesQuery,Mutation, andSubscription, dropsDrop, and runs with the expected three-line outputgo test ./internal/ltogo test ./cl -run ^TestBuildAndCheckSymbolsFromTestltoLTOPluginSharedSRet$ -count=1