Conversation
This was referenced Sep 29, 2026
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: static iface box / static itab for known T2I
This is a well-targeted change. MakeInterface now boxes compile-time-constant non-direct interface data into deduplicated read-only globals (_llgo_ifacebox$...) instead of AllocU+Store, and staticItab emits a runtime static itab global for known type-to-interface conversions even without LTO plugin markers. The runtime interequal fix correctly restores the invariant that a T2I value (static itab global) and an I2I value (NewItab) of the same interface compare equal, and it is memory-safe (both tabs are nil-guarded before dereference). The verified behaviors:
- The new comments are accurate —
staticItab's hash matchesabiCommonFields(sha256(typeName)→ little-endianuint32), and integers/bools do stayIfaceIndir. interequal's fallback short-circuits on_typemismatch, so the extraifaceeqonly runs when the type pair actually matches.- Test renames/assertions in
ssa_test.goare consistent with the new emit-always behavior.
Findings below are refinements, not blockers. The main items are two compile-time hot-path inefficiencies (expensive dedup keys computed before the cache lookup) and a couple of correctness invariants worth documenting.
Semantics note (not inline-placeable)
Boxing all constant non-direct types (structs, strings, slices, arrays, big ints) into shared GlobalConstant globals is broader than cmd/compile, which only shares addresses for the small-integer staticuint64s range and read-only static temps. Two consequences worth confirming are intended:
- Identical
(type, constant)pairs now resolve to a single shared address. Code that extracts the iface data word (viareflectinternals,//go:linkname, orunsafe) and relies on distinct addresses will observe shared identity. - Because the box is
GlobalConstant, mutating the pointee through the interface viaunsafewould now fault on a read-only page, whereas the previousAllocU+Storepath produced writable storage.
Neither is guaranteed by the Go spec, so this is likely acceptable — flagging so it's a conscious decision.
Comment on lines
+134
to
+136
| sum := sha256.Sum256([]byte(typeName + "\x00" + x.impl.String())) | ||
| name := "_llgo_ifacebox$" + base64.RawURLEncoding.EncodeToString(sum[:]) | ||
| if g := b.Pkg.VarOf(name); g != nil { |
Contributor
There was a problem hiding this comment.
[P2] staticIfaceBox: compute the expensive dedup key after the cache check
The dedup lookup b.Pkg.VarOf(name) runs after the key is built, so every constant interface conversion — including repeated conversions of a constant that is already boxed — pays x.impl.String() (serializes the full LLVM constant to textual IR, O(constant size)) plus a sha256 and base64 on every call. For programs that box the same large aggregate constant in N places, N-1 of those are cache hits that still do the full IR serialization + hash.
Consider a cheaper first-level key (e.g. the llvm.Value pointer or a small structural key) and only fall back to String()+sha256 on a miss, or memoize the computed name on the constant.
Comment on lines
+49
to
+52
| if rawIntf.NumMethods() == 0 || concrete == nil { | ||
| return Expr{}, false | ||
| } | ||
| rawIntf = rawIntf.Complete() |
Contributor
There was a problem hiding this comment.
[P2] staticItab: move the VarOf cache check before NewMethodSet/AssignableTo
rawIntf.Complete(), types.AssignableTo, types.NewMethodSet(concrete) and the per-method Lookup loop all run before the VarOf(name) cache check further down. The global name derives only from intfName/typeName (via abi.TypeName), so repeated T2I conversions of the same (interface, concrete) pair rebuild the full method set on every call only to find the itab already exists. Computing the name and checking VarOf first would make repeated conversions O(1).
| } | ||
| prog := b.Prog | ||
| typeName, _ := prog.abi.TypeName(typ.raw.Type) | ||
| sum := sha256.Sum256([]byte(typeName + "\x00" + x.impl.String())) |
Contributor
There was a problem hiding this comment.
[P3] Fragile constant identity key
The box name hashes x.impl.String() (the LLVM IR text). For constants that reference other globals — notably ConstString, whose data pointer points at an anonymous private global — the printed form can depend on module-local numbering (@0, @1, …). Distinct byte payloads printing identically is extremely unlikely today (referenced string globals are content-addressed via p.strs), but the correctness of the ODR-merged box relies on that implicit invariant. A short comment noting the assumption, or keying on the constant's actual bytes, would make this robust against future changes to anonymous/private global naming.
| if rawIntf.NumMethods() == 0 || concrete == nil { | ||
| return Expr{}, false | ||
| } | ||
| rawIntf = rawIntf.Complete() |
Contributor
There was a problem hiding this comment.
[P3] Dedup trusts abi.TypeName injectivity
Both the itab and the iface box are deduplicated by sha256(abi.TypeName(...)). If two distinct concrete types.Type values can ever map to the same abi.TypeName string, the first-emitted itab/box would be silently reused for the second, wrong type — its fun slots would point at the wrong methods, or wrong data would be shared. This is pre-existing key style, but since the itab global is now emitted in all builds (not just LTO), the blast radius is larger. Worth confirming abi.TypeName is injective for every concrete type reaching here, or folding in a stronger discriminator.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
visualfc
force-pushed
the
fix/static-iface-box
branch
from
September 30, 2026 05:18
48eba88 to
4b3a724
Compare
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
visualfc
force-pushed
the
fix/static-iface-box
branch
9 times, most recently
from
October 2, 2026 02:20
db6ed63 to
3fed6ed
Compare
visualfc
changed the title
6 tasks done
visualfc
added a commit
to visualfc/llgo
that referenced
this pull request
Oct 2, 2026
Codecov patch coverage on xgo-dev#2704 was 89.9% (target 95%). The misses were constantFingerprint's non-int kinds and staticIfaceBox's VarOf hit after the per-value cache is cleared.
Known concrete-to-interface conversions emit a read-only _llgo_itab$
global (Inter, Type, Hash, Fun) with weak_odr linkage, matching
cmd/compile's go:itab symbols.
Ordinary builds use that global as the runtime vtable. The T2I site is
the only reference, so --gc-sections/-dead_strip can drop itabs that
belong to dead functions. LTO and deadcode-drop keep calling NewItab so
unused interface methods can be dropped; Fun[] would pin them. LTO still
emits the static template for de-virt and erases unused templates after
the plugin runs.
Non-direct interface values (integers, bools, small aggregates) still
use IfaceIndir, matching Go: data is a pointer to a copy. Compile-time
constants go in a WeakODR _llgo_ifacebox$ global instead of AllocU,
like cmd/compile's static temps.
Box names hash the constant's bits, not LLVM's printed form, so
same-length strings in different packages do not collide under
Windows COMDAT. Repeated boxing of the same LLVM value reuses a
per-package pointer cache. Zero-sized constants stay AllocU so boxing
does not mutate the shared sentinel.
Together, a constant T2I is {itab, box} with no runtime call or heap
allocation. Hash is copied from the type descriptor when present.
interequal compares the (inter, _type) pair so a static itab and a
dynamically allocated itab for the same conversion compare equal.
visualfc
force-pushed
the
fix/static-iface-box
branch
from
October 2, 2026 06:43
4b91a17 to
bb2f4d3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Known concrete-to-interface (
T2I) conversions emit a read-only_llgo_itab$global ({Inter, Type, Hash, Fun[]}) withweak_odrlinkage, matching cmd/compile'sgo:itabsymbols.Ordinary builds use that global as the runtime vtable. The T2I site is the only reference, so
--gc-sections/-dead_stripcan drop itabs that belong to dead functions. LTO and deadcode-drop keep callingNewItabso unused interface methods can be dropped (Fun[]would pin them). LTO still emits the static template for de-virt and erases unused templates after the plugin runs.Non-direct interface values (integers, bools, small aggregates) stay
IfaceIndir, matching Go:datais a pointer to a copy. Compile-time constants go in aWeakODR_llgo_ifacebox$global instead ofAllocU, like cmd/compile's static temps /staticuint64s.Any LLVM-constant payload is eligible. Box names hash the constant's bits. Zero-sized constants stay
AllocUso boxing does not mutate the shared sentinel.Together, a constant
T2Iis{itab, box}with no runtime call or heap allocation.interequalcompares the(inter, _type)pair so a static itab and a dynamically allocated itab for the same conversion compare equal.Supersedes #2703 (static itabs are included in this commit).
Test plan
go test ./ssacl/_testrt/staticitab(includingany(T).(I)equality)cl/_testrt/staticifacebox(any(int64(42))has noAllocU)cl/_testrt/eface_llgo_ifacebox$dropped by-dead_strip; LTO also drops them