ssa: make recursive type conversion order-independent

[zhouguangyuan0718]

Summary

Make Go-to-raw type conversion deterministic for mutually recursive named types.

• decides whether the complete recursive graph requires conversion before publishing a recursion placeholder
• makes all back-edges observe the same converted placeholder
• preserves instantiated generic named types on recursive back-edges
• keeps conversion-free recursive graphs at their original identity
• memoizes conversion requirements to avoid repeated graph walks

This is an SSA correctness prerequisite for isolated backend Programs and does not depend on package-parallel build code.

Rebase boundary

• base: 6670dae38 (latest xgo-dev/llgo:main)
• head: 880b90308
• layer: 1 commit, 2 files, +353 / -14

Validation

• go test ./ssa -count=1
• focused recursive conversion tests under -race
• also included in the complete build: run LLVM package backends in parallel #2182 stack validation

[fennoai]

Review summary

The refactor makes named-type conversion independent of traversal order for mutually recursive types by deciding conversion need up front via a namedNeedsTypeConversion pre-pass — a sound approach, and the tests cover the key invariants (order-independence, recursive generics, per-shape detection, identity preservation). No blocking correctness defects. A few maintainability and performance points below; inline comments cover the concrete diff-line findings.

Cross-cutting notes (not tied to a single line):

• Two-switch synchronization risk. needsTypeConversion (ssa/type_cvt.go:245) re-implements the traversal that cvtType (ssa/type_cvt.go:85) performs, rather than deriving from it. The default: panic guards against new types.Type kinds, but not against a changed traversal within an existing case (e.g. if cvtStruct later converted tags, or cvtNamed started converting method signatures). The InC guard and the IsClosure short-circuit are each duplicated across both switches and must stay in lock-step. Consider a comment pointing maintainers at cvtType as the source of truth, and/or factoring the shared predicates (InC check, closure check) into helpers used by both.

• Concurrency. p.typs and the new p.cvtneed are plain maps mutated without synchronization (ssa/type_cvt.go:186,210,213,236,296), while typbg is a sync.Map. If any Program is ever driven concurrently through Type/FuncDecl/Closure, these writes race (concurrent-map-write panic). Worth confirming the single-threaded-per-Program invariant given typbg already hedges for concurrency.

[fennoai]

This second p.typs[...] = unsafe.Pointer(named) is a no-op. named is not reassigned between line 210 and here — only origin.SetUnderlying(tund) runs, which mutates the object named points to, not the map entry. Any recursive back-edge that reached t during cvtType(t.Underlying()) hit the fast path in cvtNamed and read the value already installed at line 210; it never rewrites this entry. The assignment can be removed. It's also slightly misleading: the comment above describes a single "publish before descending," but the code publishes twice.

[zhouguangyuan0718]

Addressed in ef145c2: removed the redundant second typs assignment.

[fennoai]

The origin vs named split here is load-bearing and non-obvious: for a generic instantiation, named = Instantiate(origin, t), and SetUnderlying is called on origin, not named. This works because go/types materializes the instance's underlying lazily by substituting type args into the origin's underlying — so setting the origin's underlying before named.Underlying() is first forced reflects into the returned instance. Since this is exactly what TestRecursiveGenericNamedTypeConversion guards, a one-line comment explaining why SetUnderlying targets origin (and that it must precede any named.Underlying() call) would help future readers.

[zhouguangyuan0718]

Addressed in ef145c2: added a comment documenting lazy generic-instance underlying materialization and why SetUnderlying must target origin before named.Underlying is observed.

[fennoai]

Negative results are only cached when the whole top-level query returns false (this sweep). When the top-level query returns true, conversion-free subgraphs visited along the way are never recorded as conversionNotNeeded — only the nodes on the true-returning path get conversionNeeded (line 296). So a large conversion-free named subgraph reached under a conversion-needing root stays conversionUnknown and is re-walked from scratch on every later independent query that touches it. In conversion-heavy packages (signatures/closures are common and always force conversion), this turns one-time work into repeated full-graph walks. Consider also caching conversionNotNeeded for a node whose completed underlying walk returned false (i.e. not the visiting back-edge short-circuit at line 289), which safely memoizes those subtrees.

[zhouguangyuan0718]

I am keeping the conservative negative-cache rule. Caching a node merely because its completed underlying walk returned false is unsafe when that result depends on a cycle back-edge. For B -> A -> B where B later exposes a function field, A completes false during the B-rooted query but must ultimately be converted; caching A as not-needed would recreate the original order-dependent bug. The regression that starts conversion from B covers this case. Negative states are therefore published only when the entire top-level query is negative.

[codecov]

Codecov Report

❌ Patch coverage is 95.69892% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
ssa/type_cvt.go	95.69%	3 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[github-actions]

LLGo baseline benchmarks

06b5378478de | workflow run | long-term charts

Program measurements

Platform	Workload	File size	vs base	Build	vs base	Run	vs base
Linux	cprintf	18456 B	+0.0%	230.052 ms	+0.9% (worse)	892.230 us	-6.4% (better)
Linux	fmtprintf	1829968 B	+0.0%	2.466 s	-1.5% (better)	2.420 ms	-6.0% (better)
Linux	println	68008 B	+0.0%	218.645 ms	-8.1% (better)	1.145 ms	-10.9% (better)
macOS	cprintf	84672 B	+0.0%	502.207 ms	+1.2% (worse)	3.988 ms	-2.8% (better)
macOS	fmtprintf	1869328 B	+0.0%	4.260 s	-3.1% (better)	15.431 ms	-17.3% (better)
macOS	println	121200 B	+0.0%	567.339 ms	+26.9% (worse)	12.592 ms	+151.2% (worse)

Core language and compiler benchmarks

Platform	Benchmark	ns/op	vs base
Linux	BenchmarkLookupPCRandom	9.603 ns/op	-0.1% (better)
Linux	BenchmarkMergeCompilerFlags	119 ns/op	-0.2% (better)
Linux	BenchmarkMergeLinkerFlags	80.020 ns/op	-0.6% (better)
Linux	BenchmarkChannelBuffered	49.060 ns/op	-0.6% (better)
Linux	BenchmarkChannelHandoff	28629 ns/op	+1.0% (worse)
Linux	BenchmarkDefer	37.700 ns/op	-0.4% (better)
Linux	BenchmarkDirectCall	0.968 ns/op	-9.2% (better)
Linux	BenchmarkGlobalRead	0.934 ns/op	+2.4% (worse)
Linux	BenchmarkGlobalWrite	7.246 ns/op	-0.1% (better)
Linux	BenchmarkGoroutine	37242 ns/op	+2.7% (worse)
Linux	BenchmarkInterfaceCall	6.087 ns/op	-1.1% (better)
Linux	BenchmarkRuntimeGetG	1.409 ns/op	-2.8% (better)
macOS	BenchmarkLookupPCRandom	15.530 ns/op	+7.2% (worse)
macOS	BenchmarkMergeCompilerFlags	195.300 ns/op	+41.0% (worse)
macOS	BenchmarkMergeLinkerFlags	116 ns/op	+25.1% (worse)
macOS	BenchmarkChannelBuffered	28.180 ns/op	-37.4% (better)
macOS	BenchmarkChannelHandoff	9784 ns/op	+7.1% (worse)
macOS	BenchmarkDefer	33.190 ns/op	-35.2% (better)
macOS	BenchmarkDirectCall	1.223 ns/op	-16.6% (better)
macOS	BenchmarkGlobalRead	1.092 ns/op	-18.2% (better)
macOS	BenchmarkGlobalWrite	1.254 ns/op	-14.2% (better)
macOS	BenchmarkGoroutine	55723 ns/op	-4.0% (better)
macOS	BenchmarkInterfaceCall	5.189 ns/op	-33.6% (better)
macOS	BenchmarkRuntimeGetG	2.865 ns/op	-26.1% (better)

Compared with 6670dae3884d measured in the same runner job.

[zhouguangyuan0718]

Review follow-up in ef145c2:

• merged the query bookkeeping into one map whose value carries visiting and seen;
• removed the redundant typs write;
• documented the generic origin/instance invariant;
• factored the duplicated InC decision into shouldConvertNamed and documented that the pre-pass must remain aligned with cvtType;
• documented the single-lowering-goroutine ownership of typs/cvtneed versus concurrent syntax preload writes to typbg.

Full ssa tests, focused repeated tests, race, and coverage passed locally. The conservative negative-cache behavior is intentionally retained because a completed false subtree can still depend on an unresolved cycle back-edge.

[zhouguangyuan0718]

@xgopilot please review the current PR head (880b903) from scratch. Focus on recursive type-conversion correctness, placeholder/cache state transitions, concurrent access safety, order independence and deterministic results, pointer/struct recursion, and regression-test completeness. Please ignore findings that only apply to earlier revisions.

[fennoai]

Review complete

Reviewed PR head 880b903 from scratch against base 6670dae. Findings posted as a PR review (view).

Verdict: No correctness bugs found. The order-independence fix is well-reasoned and sound.

Verified:

• Order independence / determinism — conversion decision computed over the full reachable graph and memoized in cvtneed before any placeholder is published, so it no longer depends on the entry cycle member.
• Placeholder / cache state transitions — the memoization asymmetry is correct: conversionNeeded cached inline (always sound); conversionNotNeeded cached only on a complete negative top-level query, so a back-edge's premature false is never persisted.
• cvtType / needsTypeConversion lock-step — audited all 15 type kinds; predicates match cvtType's cvt result, so the convert path can never mint a spurious duplicate named-type identity.
• Pointer/struct + generic recursion — placeholder-before-descent resolves back-edges to the converted type; generic instances derive Underlying() lazily from the origin.
• Concurrency — cvtneed is a plain map like typs, owned by the single lowering goroutine; typbg stays a sync.Map. No new shared-mutable state.

Minor findings (non-blocking):

1. Lock-step between needsTypeConversion and cvtType is a silent maintenance hazard — nothing enforces it; a future drift wouldn't panic (inline note added).
2. Test-coverage gaps: recursion through an interface method, an explicit cvtneed memoization-reuse assertion, and the InC skip path within a recursive graph.

Not verified: go test ./ssa could not run in this sandbox — the ssa package requires LLVM C headers (llvm-c/Core.h) via github.com/xgo-dev/llvm, which aren't installed, so the package doesn't build here. The type-conversion logic is pure go/types and was reviewed by hand; rely on the PR's own go test ./ssa -count=1 / -race runs for execution evidence.

View job run

[fennoai]

Review: recursive type conversion, order-independent

Reviewed PR head 880b903 from scratch against base 6670dae, focused on the areas requested. No correctness bugs found. The change is well-reasoned and the approach is sound. Findings below are minor (a maintenance risk and test-coverage gaps).

What the change does (and why it's correct)

The old cvtNamed installed the original type as the recursion placeholder and only swapped in the converted type if the underlying descent reported cvt==true. For mutually recursive named types this made the result depend on which cycle member was converted first. The new flow fixes that cleanly:

1. namedNeedsTypeConversion(t) decides up-front, over the whole reachable graph, whether conversion is required — memoized in cvtneed, so the decision is independent of entry point (true order independence).
2. If not needed → cache the original identity and return (t, false) (conversion-free recursive graphs keep their identity — verified by TestRecursiveNamedTypesWithoutConversionKeepTheirIdentity).
3. If needed → build the converted named/origin, publish it as the placeholder before descending, then origin.SetUnderlying(tund). Every back-edge now observes the same converted placeholder.

I verified the following:

• Order independence / determinism — the conversion decision is computed for the full graph and cached in cvtneed before any placeholder is published; TestNamedTypeConversionIsIndependentOfTraversalOrder covers A-first vs B-first over a struct+pointer cycle containing a closure.
• Placeholder / cache state transitions — the memoization asymmetry is correct: conversionNeeded is cached inline during traversal (a positive result is always sound), while conversionNotNeeded is cached only when the complete top-level query is negative (a negative observed on a cycle back-edge alone is never stored). This correctly avoids caching a "false" that was contaminated by a premature back-edge return.
• cvtType / needsTypeConversion lock-step — audited all 15 type kinds; each predicate matches what cvtType would set for cvt, including the top-level opaque-type check, IsClosure short-circuits, Signature/Union always-true, Interface (params + results of each explicit method, plus embeddeds), and TypeParam/Alias. Because they match, the namedNeedsTypeConversion==true path can never publish a converted placeholder whose underlying descent then returns cvt==false (which would have minted a spurious duplicate identity).
• Pointer/struct recursion — placeholder-before-descent makes back-edges resolve to the converted type; generic instances resolve named.Underlying() lazily from origin, so a recursive My[T] back-edge lands on the converted My[args] instance (TestRecursiveGenericNamedTypeConversion asserts next == raw).
• Concurrency safety — cvtneed is a plain map, consistent with typs already being a plain map; the doc comment states both are owned by the single lowering goroutine while typbg stays a sync.Map because it is populated during concurrent preloading before lowering. This introduces no new shared-mutable state beyond the existing model, so no new data race — provided the single-owner invariant holds during lowering.

Findings (minor)

1. Lock-step is a silent maintenance hazard (informational). needsTypeConversion must stay exactly in step with cvtType; the code documents this but nothing enforces it. If a future edit adds a convertible case to cvtType (or changes cvtUnion/cvtClosure to sometimes be a no-op) without updating needsTypeConversion, the two can silently disagree — a true predicate with a no-op descent would mint a spurious duplicate named-type identity, and a false predicate with a real conversion would skip conversion. Neither would panic. Consider a small assertion path in tests, or a shared traversal, to guard against drift. (Inline note added.)

2. Test-coverage gaps (minor). Current tests are strong for pointer/struct cycles, generic recursion, per-kind requirement shapes, and identity preservation. Not directly covered:

• A named type that recurses through an interface method (rather than a pointer/struct field) — exercises the Interface branch of needsTypeConversion inside a cycle.
• An explicit assertion that the cvtneed memoization is reused across two separate cvtNamed/namedNeedsTypeConversion calls (the perf/determinism claim of the PR), rather than only implied.
• The InC skip path (shouldConvertNamed==false) interacting with a recursive graph.

These are optional hardening, not blockers.

Verification note

I could not execute go test ./ssa in this sandbox: the ssa package transitively requires the LLVM C headers (llvm-c/Core.h, PassBuilder.h) via github.com/xgo-dev/llvm, which are not installed here, so the package does not build. The type-conversion logic itself is pure go/types and was reviewed by hand. Please rely on the PR's own go test ./ssa -count=1 and -race runs for execution evidence.

[fennoai]

Maintenance hazard: needsTypeConversion must stay exactly in lock-step with cvtType, but nothing enforces it. If a future change adds a convertible case to cvtType (or makes cvtUnion/cvtClosure a no-op in some case) without mirroring it here, the two silently diverge:

• predicate true + no-op descent → cvtNamed publishes a converted placeholder whose underlying equals the original, minting a spurious duplicate named-type identity;
• predicate false + real conversion → conversion is skipped entirely.

Neither case panics, so drift would be hard to catch. Consider a shared traversal or a test that asserts needsTypeConversion(x) == (cvtType(x) changed) across a representative type corpus to guard against this.