Repository navigation
simd: enable portable 256-bit variants on amd64 - #3049
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
SIMD256 portable Midway review
This PR extends LLGo's Midway specialization stage from two width variants (emulated + 128-bit) to three on amd64 (adding a 256-bit / AVX2 variant), threads the target arch through Rewrite, generalizes the scalar-entry dispatch to select by simd.VectorBitSize(), updates the runtime width policy (archMaxVectorSize, checkMidwayWidth), and teaches the recover dispatcher about the @simd256 sibling.
Overall this is a clean, well-tested change. arch is threaded consistently end-to-end (conf.Goarch → rewritePortableSIMD → midway.Rewrite → targetWidths), the dispatch ordering (emulated → width-equality → 128 fallback) is correct given that emulation reports a logical width of 128, the width-selection trust boundary stays memory-safe (any non-256 width falls through to the 128 baseline, and only generated variants can ever be selected), and the new tests cover the positive/negative cases well (per-target variant presence/absence, FMV-composed 256 variants, the partialWide recover negative case, and runtime rejection of unsupported widths). No correctness or safety issues were found.
The notes below are all minor / maintainability.
Stale documentation outside the diff
test/simd/README.md:188-190 (just below the hunk this PR edits) still says an external storage binding "needs an ABI shared by both concrete representations" and that embedded strings/byte slices "retain their contents in both variants." With the 256-bit variant added, amd64 now has three concrete representations (@simd0, @simd128, @simd256), and global() replicates embed-bearing globals across every entry in f.widths. Consider generalizing "both" → "all" / "every" while this section is being updated. (Flagged here rather than inline because those exact lines fall outside the PR diff.)
| stmts = append(stmts, &ast.IfStmt{If: pos, Cond: cond, | ||
| Body: &ast.BlockStmt{Lbrace: pos, Rbrace: pos, List: invoke(width)}}) | ||
| } | ||
| return append(stmts, invoke(128)...) |
There was a problem hiding this comment.
dispatchWidth hardcodes 128 twice — as the case 128: continue skip and as the trailing append(stmts, invoke(128)...) fallback. This implicitly assumes 128 is always present in f.widths and is always the correct final fallback. Both hold today (targetWidths always includes 128), but the literal is disconnected from targetWidths: if a future target ever dropped 128 or used a different baseline, this would emit a dispatcher calling a non-existent @simd128 variant with no compile-time signal. Consider a named const fallbackWidth = 128 (shared with targetWidths) or a comment documenting the invariant that targetWidths must always contain the fallback width.
| return 0, 0 | ||
| if archsimd.X86.AVX2() { | ||
| size = 256 | ||
| if cpu.X86.HasVPCLMULQDQ { |
There was a problem hiding this comment.
The else-less VPCLMULQDQ logic relies on the preceding AVX branch having already set allFeatureSize = 128, so that when AVX2 is present but HasVPCLMULQDQ is false, allFeatureSize correctly stays 128 rather than 0. This is safe (AVX2 implies AVX, so the AVX branch always runs here), but the dependency is implicit. A one-line comment — e.g. "AVX2 implies AVX, so allFeatureSize is already 128" — would make this self-documenting and guard against a later reorder/split of the two branches.
| func checkMidwayWidth() { | ||
| if maxVectorSize != 128 { | ||
| panic("LLGo portable SIMD supports emulation and 128-bit vectors only") | ||
| if maxVectorSize != 128 && (runtime.GOARCH != "amd64" || maxVectorSize != 256) { |
There was a problem hiding this comment.
The amd64/256 width policy is encoded here (accept 256 only on amd64) and independently in internal/midway/analysis.go targetWidths ({0,128,256} for amd64). These must stay in lockstep — a runtime-selectable width must have a corresponding generated variant — but nothing links the two sites. A brief cross-reference comment in one pointing to the other would reduce drift risk if, e.g., arm64 later gains 256-bit variants. (Optional: the condition maxVectorSize != 128 && (runtime.GOARCH != "amd64" || maxVectorSize != 256) is correct but reads as an inverted De Morgan form; a positive supported := maxVectorSize == 128 || (GOARCH == "amd64" && maxVectorSize == 256) would be easier to read.)
Enable portable
simdprograms to select 256-bit variants on amd64. Midway specializes declarations and cross-package references, shares dispatch between calls and global initialization, and preserves deferredrecoverthrough every generated variant. Runtime selection follows Go's effective AVX/AVX2 and VPCLMULQDQ policy, capped at 256 bits; ARM64 and Wasm retain their existing widths.Adds four-target FileCheck coverage for dispatch, vector operations and recovery frames, plus 27 upstream Go tests and 6 examples in
test/std/simd. Native CI also runs 72 qualified ARM64 and 125 qualified AMD64 instruction tests directly from GOROOT. Unsupported and hardware-gated cases are documented indev/simd/README.md; the two inner-product tests explicitly skip LLGo's unsupported 256-bit reduction helper while retaining their emulated/128-bit coverage.Validation: focused compiler tests, the full native ARM64 SIMD script, Linux AMD64 O0/O2 emulation at 0/128/256 widths and CPU overrides, and WASI/GoJS O2 execution passed. Hosted CI is rerunning on the latest commit; local emulation is not native x86 qualification.