Conversation
2 tasks done
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: fold trivial constant interface boxing
Solid optimization overall — the static itab / static ifacebox globals replace runtime NewItab and AllocU for compile-time constants, and the interequal fix correctly keeps T2I (static itab) and I2I (NewItab) representations of the same iface equal by comparing the (inter, _type) pair with proper nil guards. Verified: the removed LTO/compiler.used template comments are no longer stale, the staticItab hash matches the concrete type descriptor, and boxed constants are safely immutable under Go semantics (assertions copy out; no addressable path reaches the read-only box).
Two correctness concerns worth addressing before merge, plus one minor doc nit — inline.
| case *ssa.ChangeType: | ||
| chain[t] = true | ||
| v = t.X | ||
| case *ssa.Convert: |
Contributor
There was a problem hiding this comment.
[P1] Convert in fold chain reinterprets the raw constant under the wrong type
analyzeTrivialIfaceBox accepts *ssa.Convert in the chain and returns concrete = mi.X.Type() (the type after the conversion), while foldConstantMakeValue then applies the callee argument's raw constant via b.Const(c.Value, concrete) at cl/constiface.go:50 — skipping the conversion arithmetic.
This is correct only for representation-preserving conversions. For a value-changing Convert it is unsound. Example: func box(x int32) any { return any(string(x)) } called as box(65). Here concrete is string but c.Value is the integer 65; b.Const dispatches on the target kind (ssa/expr.go), hits the types.String branch, and calls constant.StringVal on an integer constant — producing a wrong result or a panic, where real Go yields any("A"). Numeric narrowing/widening and int↔float Convert have the same hazard.
*ssa.Convert in go/ssa is exactly the set of value-changing conversions, so including it here is unsafe. Suggest dropping *ssa.Convert from the recognized chain (keeping only ChangeType/ChangeInterface, which are representation-identical), or skip folding whenever a Convert is present.
| if g.impl.IsNil() { | ||
| return llvm.Value{}, false | ||
| } | ||
| g.impl.SetGlobalConstant(true) |
Contributor
There was a problem hiding this comment.
[P1] staticIfaceBox mutates the shared zero-sized sentinel for zero-sized types
For a zero-sized value type (e.g. struct{}, [0]T, or a named type over them — none are directIfaceType, so MakeInterface routes them here), NewVarEx → doNewVarEx does not create a dedicated global. It returns the shared module zero-sized sentinel (__llgo.moduleZeroSizedAlloc$) as an isZeroSizedAlias Global (ssa/decl.go:209-219).
g.Init(x) is a no-op for that alias, but the following lines are not guarded and run on the shared sentinel:
g.impl.SetGlobalConstant(true)
g.impl.SetUnnamedAddr(true)
b.Pkg.setODRLinkage(g.impl, llvm.WeakODRLinkage)Boxing any zero-sized constant thus flips the shared sentinel to GlobalConstant and rewrites its linkage (from LinkOnceODRLinkage, or re-COMDATs it on Windows) — a cross-cutting side effect on the address handed out for all zero-sized allocations in the module. It also pollutes the _llgo_ifacebox$ dedup cache with an entry pointing at the sentinel.
Suggest bailing out (return false) before the constant/linkage mutations when the resulting Global is a zero-sized alias — the isZeroSizedAlias field already exists on aGlobal, or guard on TypeAllocSize(storageType)==0. Zero-sized boxes don't benefit from this optimization anyway.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
visualfc
force-pushed
the
fix/trivial-iface-fold
branch
from
September 30, 2026 05:18
b0f449e to
aaa3c6e
Compare
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
visualfc
force-pushed
the
fix/trivial-iface-fold
branch
2 times, most recently
from
September 30, 2026 12:54
2780773 to
347be55
Compare
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. 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.
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.
Depends on static itabs so a constant T2I can be {itab, box} with no
runtime call.
A call whose SSA body is MakeInterface of a Convert/ChangeType of a
constant parameter is lowered at the call site to MakeInterface of
that concrete value. No function-name matching: constant.MakeInt64
and user helpers such as boxMyInt(x int64) any { return myInt(x) }
use the same path.
Depends on static itabs and iface boxes so the folded value is a
compile-time {itab, box} pair.
visualfc
force-pushed
the
fix/trivial-iface-fold
branch
from
September 30, 2026 14:19
347be55 to
d0f5383
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
A call whose SSA body is
MakeInterfaceof aConvert/ChangeTypeof a constant parameter is lowered at the call site toMakeInterfaceof that concrete value.The callee is recognized by that SSA shape (single block, no
//go:noinline), not by package path or function name.constant.MakeInt64/MakeBoolmatch because they arereturn int64Val(x)/return boolVal(b). User helpers such asfunc boxMyInt(x int64) any { return myInt(x) }use the same path. Functions with extra control flow (MakeString) are left as real calls.Depends on: #2703 (static itabs) and #2704 (iface boxes). Without those, this fold would emit
AllocU+NewItabper call. Merge after #2704. This branch contains #2703–#2704 plus this commit; only the latest commit is in scope.Test plan
cl/_testrt/constiface(MakeInt64,MakeBool, and a localboxMyIntall become{itab, box})