Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: static maplit + static itab/interface boxing
This is a well-structured change. The interface-representation split (T2I now uses static itab globals while I2I still goes through NewItab) is handled carefully, and the interequal fix correctly restores cross-representation equality. interhash hashes by tab._type, and both itab representations point _type/hash at the same ODR-merged type descriptor, so map insert/lookup stays consistent. The maplit-to-loop lowering is safe by construction (keys required constant, arrays synthesized at known length).
Findings below are mostly correctness-hardening and readability; none are blocking.
Verified sound: full 32-byte sha256 in box/itab names (no truncation collision), mapLitIndexAddr only indexes fixed-length synthesized arrays, static globals are SetGlobalConstant(true) and never mutated after Init, and the staticItab.hash computation matches abiCommonFields.
| if classifyStaticComplit(updates, plan) { | ||
| plan.kind = mapLitStaticComplit | ||
| } else { | ||
| plan.skip = nil |
Contributor
There was a problem hiding this comment.
[P2] classifyStaticComplit mutates plan on failure; plan.skip=nil is misleading
classifyStaticComplit mutates the passed plan (sets plan.structTy at the top, appends to skip) before it may return false on a later entry. After a false return, plan.structTy is left populated while plan.kind stays mapLitGeneric. The else { plan.skip = nil } here only resets one of the two mutated fields, and plan.skip is already nil on a freshly-allocated plan, so the line is both dead and misleading. This works today only because the generic branch of compileMapLitUpdate never reads plan.structTy — a fragile invariant. Suggest making classifyStaticComplit compute structTy/skip in locals and commit to plan only on success (return them, assign in the caller), removing the need for plan.skip = nil.
| return false | ||
| } | ||
| if block != afterBlock { | ||
| return block.Index > afterBlock.Index |
Contributor
There was a problem hiding this comment.
[P2] instrIsAfter treats BasicBlock.Index as execution order for all functions
instrIsAfter uses block.Index > afterBlock.Index as "executes after". Block index is the position in fn.Blocks (roughly RPO for x/tools SSA) and does not guarantee execution/dominance order across branches. collectLargeMapLits runs for every function (compile.go:759), not just the synthetic package initializer. For a large map literal inside a normal function with branching, a makeMap referrer in a higher-indexed block that is actually on a parallel/earlier path could be wrongly judged "after" the last update, permitting an unsafe delay. The sibling static-map-init pass gates on initFn.Synthetic == "package initializer". Consider restricting this optimization to synthetic init functions, or strengthening the ordering check (e.g. real dominance) and documenting the assumption.
| if !ok || c.Value == nil { | ||
| return llssa.Expr{}, false | ||
| } | ||
| x := b.Const(c.Value, p.type_(concrete, llssa.InGo)) |
Contributor
There was a problem hiding this comment.
[P2] Constant re-typing in foldConstantMakeValue may skip Convert semantics
analyzeTrivialIfaceBox accepts a *ssa.Convert in the unwrap chain and returns mi.X.Type() as the concrete type. Here the source constant c has the parameter's type, but the value is materialized directly at concrete (= mi.X.Type()). If the chain contains a real numeric Convert (e.g. param int32 widened/narrowed to int64, or a representation-changing conversion), building b.Const(c.Value, concrete) reinterprets the untyped constant instead of applying the conversion, which can diverge from actually calling the constructor. ChangeType/ChangeInterface are representation-preserving and safe. Consider rejecting *ssa.Convert in the accepted chain (or applying the conversion to the constant). Low likelihood in practice, but worth hardening.
| rawIntf.NumMethods() == 0 || concrete == nil { | ||
| if rawIntf.NumMethods() == 0 || concrete == nil { | ||
| return Expr{}, false | ||
| } |
Contributor
There was a problem hiding this comment.
[P3] staticItab does method-set work before the dedup cache check
staticItab computes types.NewMethodSet(concrete) plus per-method Lookup and AssignableTo before it builds the cache key name and checks b.Pkg.VarOf(name). Since the itab is deduplicated per (interface, concrete) pair and the cache key does not need the method set, repeated boxings of the same pair recompute the full method set only to discard it on the cache hit. Hoisting the VarOf(name) early-return above the method-set work would make repeated boxings O(1).
| return !v.impl.IsNil() && !v.impl.IsAConstant().IsNil() | ||
| } | ||
|
|
||
| var mapLitSeq atomic.Uint64 |
Contributor
There was a problem hiding this comment.
[P3] mapLitArray globals use a non-dedup, order-dependent counter
mapLitArray names emitted constant arrays via a process-global atomic.Uint64 (mapLitSeq), unlike the other two globals in this PR (staticItab, staticIfaceBox) which are content-addressed and thus deduplicated + deterministic. Consequences: identical constant map-literal arrays never merge (code/data-size bloat), and names depend on compilation order, hurting build reproducibility under parallel/reordered compilation. Content-hashing the array would fix both.
| if x.tab == y.tab { | ||
| return ifaceeq(x.tab, x.data, y.data) | ||
| } | ||
| // T2I uses a static itab global; I2I still goes through NewItab. |
Contributor
There was a problem hiding this comment.
[P3] interequal/foldConstantMakeValue comments slightly overstate the mapping
Two minor doc nits: (1) alg.go's // T2I uses a static itab global; I2I still goes through NewItab — T2I falls back to NewItab too when staticItab returns false (empty interface, unassignable, no-interface-method), so it is not a strict one-to-one mapping; the equality logic is still correct. (2) foldConstantMakeValue's doc lists Convert/ChangeType but the recognizer (analyzeTrivialIfaceBox) also accepts ChangeInterface; align the two comments.
| } | ||
| g := b.Pkg.NewVarEx(name, prog.Pointer(typ)) | ||
| g.Init(x) | ||
| if g.impl.IsNil() { |
Contributor
There was a problem hiding this comment.
[P3] staticIfaceBox: potential leftover global if Init fails
If g.Init(x) leaves g.impl nil, the function returns (zero, false), but the named global was already created via NewVarEx. A later call with the same name would hit the VarOf(name) cache and return that leftover, uninitialized global with true. Confirm NewVarEx does not register the var when init fails, or clean up on the failure path. Also worth a one-line note that x.impl.String() is used purely as a dedup key so correctness does not depend on LLVM's textual-print stability.
Codecov Report❌ Patch coverage is
📢 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/static-maplit
branch
from
September 30, 2026 05:18
33a5cef to
ab860f1
Compare
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
visualfc
force-pushed
the
fix/static-maplit
branch
4 times, most recently
from
October 2, 2026 02:23
1c5017c to
8d946e2
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.
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.
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.
Map literals with more than 25 constant keys become a counted
mapassign loop over private constant key/value arrays, matching
cmd/compile's maplit. Composite values whose fields are constants or
trivial iface constructors (PR 3) are rebuilt as LLVM constants so
the value array is a ConstArray.
Runtime values stay unrolled. Spilling them into alloca [N x T] makes
LLVM default<Os> SLP scalarize the array.
Depends on trivial iface folding so {string, MakeInt64/MakeBool}
entries are compile-time structs.
visualfc
force-pushed
the
fix/static-maplit
branch
from
October 2, 2026 06:45
8d946e2 to
b08ce43
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
Map literals with more than 25 constant keys become a counted
mapassignloop over private constant key/value arrays, matching cmd/compile'smaplit.Composite values whose fields are constants or trivial iface constructors (the SSA shape from #2705) are rebuilt as LLVM constants so the value array is a
ConstArray. MixedMakeInt64/MakeBool-style boxes in one literal are allowed. Identification is by SSA (MakeMap, constant keys, complit fields), not by function name.Depends on: #2704 and #2705. The large
{string, constant.Value}literals need folded constant ifaces. Merge after #2705. This branch contains #2704–#2705 plus this commit; only the latest commit is in scope.Test plan
go test ./cl -run TestCollectcl/_testrt/maplit(map[string]intand mixed int/bool complits)