Repository navigation
compiler: add portable SIMD Midway specialization - #2819
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: portable SIMD Midway specialization
This is a carefully engineered first Midway pass. The two-pass type-check integration in the loader is clean, the tricky cases are handled deliberately (recursive aggregates, aliases, generic constraints, embedded fields, import shadowing, init-order edges), and the comments are genuinely helpful. The runtime representation choice is fail-loud (panic on an unsupported width) rather than a silent miscompile, and the "undefined portable symbol" bug class is correctly closed: bodyless SIMD-dependent functions error cleanly and the re-type-check rejects malformed output. The @-prefixed synthetic identifiers were verified safe against the init/stdcall name-detection sites.
I ran down the one potentially-critical finding and disproved it: a package-level blank var (var _ = simd.Foo()) does not nil-deref in global() — go/types records package-level _ names as non-nil *types.Var (confirmed by compiling a probe), so that path is safe. It is dropped from this review.
Remaining findings are robustness / clarity notes and compile-time performance observations; none block merge. All are well-gated by importsSIMD, so non-SIMD packages pay nothing.
Not blocking — notes only:
-
Error accumulation is last-write-wins.
typeExprwritesf.errat several sites (internal/midway/types.go:24,81,112,136) and keeps building a knowingly-invalid tree;f.err/r.erris only read once per file atrewrite.go:139. For a struct/interface with multiple foreign-unexported members, only the last offending member is reported. Correctness currently holds because the tree isn't used after an error returns, but it couples to that ordering. Consider failing fast or accumulating. -
In-place AST mutation on the error path.
Rewritemutatesfile.Declsand decl bodies in place while iterating, so an error after some files were processed leavespkg.Syntaxinconsistent with the still-originalpkg.Types/pkg.TypesInfo. This is contained —appendErrorsetsIllTyped, which skips the cache store (load.go:377) and propagates — but the invariant is worth a one-line comment since safety depends entirely onIllTypedshort-circuiting every downstream consumer. -
Comment nit:
rewrite.go:60-61says "Only the publicFromArchgeneric functions" but the guard matches thetofrom_filename prefix, which also coversToArch. The code is correct; the comment under-describes it (the README correctly lists both).
See inline comments for the arch-cap and compile-time-complexity observations.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
e3c5644 to
3424b61
Compare
|
Rebased onto main The non-inline notes are also addressed:
Added inferred-aggregate type reconstruction checks; local |
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 |
|
Reviewed commit
Validation: the existing |
|
Follow-up review of the unchanged head
There is also stronger evidence for the previously reported global-initializer grouping issue: it can silently reorder side effects, rather than only reporting a false cycle. For this program: package main
import "simd"
var events []int
func vector() simd.Uint32s {
events = append(events, 1)
return simd.BroadcastUint32s(1)
}
func scalar() int { events = append(events, 2); return 42 }
var A, B = vector(), C
var C = scalar()
func main() { println(events[0], events[1], B) }WASI O0 prints Validation: all four additional failures were reproduced through the actual PR compiler. Original source acceptance was also checked with the frontend type checker. Additional rewrite/recheck controls for grouped constants, named arrays/aliases, comma-ok map reads and assertions, type switches, ordinary generic functions, inferred anonymous results, and local types passed. Go 1.27's experimental SIMD compiler also fails the generic/inferred-global examples, so those runs were not treated as passing semantic controls. Native runtime execution remains blocked by the local SDK/LLD incompatibility; execution evidence above is from Wasmer. |
|
Addressed the six correctness issues from the initial review and the follow-up in ca6a62d.
Validation: all reported failure classes were independently reproduced on 97797ec. After the fix, 14 probe/control builds and 28 WASI/Wasmer executions passed. The portable suite passed on Darwin/arm64 and WASI/Wasmer under O0, O2, ThinLTO, and FullLTO, each with Hosted CI for the new commit is pending. The separately prepared FMV bridge-eligibility fix and entry-fact/variant-pruning work remain outside this update. |
cpunion
left a comment
cpunion
left a comment
There was a problem hiding this comment.
Reviewed ca6a62d2c8709a012757bc98e32724293618628a. Two additional silent data-correctness issues remain in global specialization. Both were reproduced through the PR compiler at O0/O2 on WASI with hardware and emulated selection; corresponding controls behave correctly. The existing Midway/package-loader suites, four-target Midway LLVM checks, metadata checks, and recovery-classification regression passed.
| extra = append(extra, &ast.GenDecl{TokPos: pos, Tok: token.VAR, Specs: []ast.Spec{&ast.ValueSpec{ | ||
| Names: []*ast.Ident{ident(id.Name+suffix(width), pos)}, Type: f.typeExpr(obj.Type(), width, pos), | ||
| }}}) |
There was a problem hiding this comment.
[P2] Initialize embedded data in the concrete global variants
The new declarations have no GenDecl.Doc or ValueSpec.Doc, so go:embed initializes only the retained original variable. Executable code reads data@simd0 / data@simd128, which stay zero-valued. This also affects named scalar types whose only SIMD dependency is a method body:
package main
import (_ "embed"; "simd")
type Data string
func (Data) Width() int { var v simd.Uint32s; return v.Len() }
//go:embed text.txt
var data Data
func main() { println("data", string(data), "width", data.Width()) }With text.txt containing embedded data, the result is data width 4, instead of data embedded data width 4. Replacing Width's body with return 4 restores the embedded contents. Reproduced with the PR compiler on WASI at O0 and O2, under both GODEBUG=simd=128 and simd=0; also reproduced on Darwin/arm64 at O0. Please preserve embed initialization for the concrete storage and add an execution regression.
There was a problem hiding this comment.
Verified on 851907a0ac00846e5f73fa95208673344279d6ad: the original named-string reproducer now prints data embedded data width 4 in both hardware and emulated modes. The named-string/byte-slice execution regressions also pass on Darwin/arm64 and WASI, including initialization-time reads. I separately reported the new fixture's Windows CRLF checkout failure in #2819 (comment); that failure is a test expectation issue, not lost embed data.
| if packageObject(obj) && f.a.dependent(obj) { | ||
| n.Name += suffix(width) |
There was a problem hiding this comment.
[P2] Preserve global linkname aliases when rewriting their references
For a SIMD-dependent variable, this rewrites an alias reference to Alias@simd0 / Alias@simd128. However, the go:linkname relationship remains on the retained original declaration, while global() allocates the alias variants as independent zero-valued globals. The alias consequently stops observing its target's contents:
package main
import ("simd"; _ "unsafe")
var V = simd.BroadcastUint32s(7)
//go:linkname Alias main.V
var Alias simd.Uint32s
func main() {
out := make([]uint32, 4)
Alias.Store(out)
println(out[0])
}This builds successfully and prints 0 instead of 7. Reproduced on WASI at O0 and O2 with both GODEBUG=simd=128 and simd=0. A second probe using a named integer with a SIMD-dependent method prints Alias=0, V=7; its non-SIMD control prints Alias=7, V=7, confirming this is an alias-storage regression rather than a vector-store issue. Please preserve the alias relationship between the corresponding concrete variants, or diagnose an unsupported binding before generating code.
There was a problem hiding this comment.
Verified on 851907a0ac00846e5f73fa95208673344279d6ad: both original SIMD alias reproducers now fail during compilation with cannot specialize go:linkname binding for SIMD-dependent global Alias, while the non-SIMD alias control still prints alias 7 V 7 4. The new same-package/imported-target diagnostic tests pass too. This resolves the silent wrong-storage behavior by explicitly rejecting the unsupported binding; it does not add SIMD global-linkname support.
| var embeddedAtInit = string(midwayEmbeddedText) + string(midwayEmbeddedBytes) | ||
|
|
||
| func TestPortableEmbeddedGlobals(t *testing.T) { | ||
| const want = "embedded data\n" |
There was a problem hiding this comment.
[P2] Preserve the fixture's line endings or avoid a fixed LF expectation
The new text fixture has no eol=lf/binary attribute, so Windows checkouts convert it to CRLF. Embedding correctly preserves those bytes, but this fixed expectation makes the portable SIMD suite fail. The latest Windows MinGW/amd64 job already fails here: job log reports text="embedded data\r\n", bytes="embedded data\r\n", and the corresponding doubled initializer string.
I also reproduced the same failure on Darwin/arm64 with this exact compiler revision by copying the portable test package, changing only testdata/embedded.txt to embedded data\r\n, and running llgo test -O0 -run '^TestPortableEmbeddedGlobals$'. The LF checkout passes. Please pin the fixture's bytes in .gitattributes (as the existing golden files do), or derive the expected content without assuming checkout line endings, so the new regression does not break the Windows SIMD matrix.
There was a problem hiding this comment.
Verified fixed in fac37d4e2ef9449a04256daeb132555dabc53757. A fresh git -c core.autocrlf=true checkout-index of the updated fixture produces exactly b'embedded data', with no line ending to convert. I copied the current portable test package into that checkout and ran llgo test -O0 -v -count=1 -run '^TestPortableEmbeddedGlobals$' using a compiler built from this exact commit; it passes. The latest Midway/package-loader suites and four-target code-generation/global-binding regressions also pass. This verifies the fixture fix locally; the new Windows CI jobs are still pending.
Portable
simdprograms currently reach undefined portable method symbols even though the SIMD128 bridge is implemented. This adds LLGo's first Midway pass: type-check source, generate emulated and 128-bit declarations, rewrite references to the official bridge, then type-check again before constructing Go SSA. Portable reductions run with the default hardware representation orGODEBUG=simd=0.Based on main
762d7bd73a343f2c8815b45e77c461d850ea45c7, including #2799, #2802, and the GC fixture repair #2821. The GC repair is no longer part of this PR's diff. Related to #2568; wider variants and general FMV remain follow-ups.Implementation
initdeclarations execute in order. Retained vector signatures trap if reached without specialization. Carry embed directives onto concrete global storage, including named strings and byte slices. Diagnosego:linkname,llgo:link, andexportbindings on SIMD-dependent globals, including known targets using the explicitgo:namespace, before generating unrelated alias storage. Blank functions and methods are checked but do not generate variants or seed receiver specialization.IllTypedand are excluded from the loader cache.Validation
Latest cleanup and boundary audit:
1 2 42.llgo:link, exported variables, and known targets written withgo:; these previously produced detached zero-valued alias storage. New boundary probes passed 28 WASI executions, while the unsupported bindings were rejected at O0/O2.core.autocrlf=truecheckout filter preserves the 13-byte fixture; this addresses the Windows CRLF test failures without normalizing or weakening the embedded-content assertions.comparablebridge alias. Generic declarations still use syntax cloning; valid generic and inferred-aggregate regressions remain covered.internal/midway,internal/packages, andclsuites passed, along with four-targetTestSIMDMidwayLLVM, metadata and dependency-closure tests, and six build-level binding rejection cases. The shared portable tests also passed with official Go.simd=0/128: 16 suite runs and 192 test passes. Formatting and diff checks passed.Earlier local coverage includes Linux/amd64 CPU overrides and the GC boundary regression after rebasing onto main. Those execution checks were not repeated for this update. Local JavaScript execution remains unavailable without
emcc; wasm LLVM/code generation and WASI execution passed. Hosted CI for the new commit is pending. The separate Midway/FMV bridge-eligibility change and entry-fact/variant-pruning work are not included in this update.Scope limits
This stage emits emulated and SIMD128 variants only. Reflection-driven portable calls, bodyless portable-dependent user functions, and inferred globals requiring another package's unexported type/field/method names are outside this first stage; the latter two forms report source diagnostics. Global external-symbol bindings involving specialized storage are not supported: Midway diagnoses dependent source variables (
go:linkname,llgo:link, orexport) and known dependent targets instead of generating independent zero-valued alias globals. LLGo-specific initialization/embedding fixtures are separate from official Go controls because Go 1.27.0's Midway rejects keyed embedded portable fields and lacks the same global-initializer handling. These are compiler correctness checks, not runtime performance results.