Repository navigation
simd: preserve native C vector types and ARM64 aggregate ABI - #2841
zhouguangyuan0718 wants to merge 4 commits into
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: preserve native C vector types and aggregate ABI
This is a well-structured change with strong test coverage (source-vs-LLVM layout agreement, HVA spilling, callbacks/exports, array/union/alignment diagnostics). The security pass found no real issues — Clang is invoked via argv (no shell injection), temp files are cleaned up on every error path, and the unsafe.Pointer conversions follow the existing goProgram/aProgram layout-compatible pattern. The panics in cVectorArray are defensible as internal invariant assertions.
The findings below are the ones worth acting on — the two performance items affect every compiled package, not just vector-using ones. Everything is non-blocking.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
9cd00e3 to
c3b9a21
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 |
|
@fennoai /review Please re-review the latest head Added an atomic metadata guard for vector source-layout traversal, a failing-before/passing-after allocation regression, native-registration coverage, and clearer source-position/lane-count contracts. Kept the conservative C array prefilter to preserve unsupported-vector diagnostics; the inline reply explains why a 16-byte-only filter is unsafe. Full SSA tests, C vector build/diagnostic tests, and native C interop pass locally. Please pay particular attention to source layout, shared metadata and ABI boundaries. The previous inline findings have individual responses and dispositions. All three SIMD PRs also include the isolated native-debug harness fix from commit 2cc3c5b in #2849: disable external symbol lookup and stream LLDB progress. Their preceding Ubuntu jobs exhausted the job limit in native source-debug acceptance after SIMD tests passed; the fixed harness passes locally. The preceding coverage checks passed (this is not a claim about the fresh CI run). Fresh CI has been triggered for this head and is still pending. Please report any remaining correctness, regression, or performance issues against this exact revision. Review only; no code changes are requested. |
Re-review at
|
There was a problem hiding this comment.
Re-review at 1b65c4b — follow-up changes verified
I re-reviewed the full PR diff against main plus the follow-up commits since my last pass (c3b9a21d1, 1b65c4b9f), with four subagents focused on source layout, shared metadata, and ABI boundaries. Two subagents independently converged on the one item below.
Previous findings — all addressed:
- [P2] vector-layout traversal cost → Fixed. The new
hasVectorLayouts atomic.Boollatch (ssa/package_syntax.go), set inSetCVectorType/SetNativeStructFields/SetTypeCallConvand checked first invectorLayout/vectorStructLayout, mirrors the existinghasStructLayoutspattern and restores the zero-allocation common path. The monotonic false→true latch (atomic store undermu, lock-free load) is race-free, andTestOrdinarySourceLayoutAllocationsis a valid allocation-equality regression guard. - [P2] conservative
possibleVectorClang gate → Correctly kept. I confirmed a 16-byte-only prefilter would silently suppress the mandatory "unsupported vector" diagnostic for wider vectors (avector_size(32)typedef arrives as a 32-byte Go array and would never reach Clang). The added comment accurately explains why. - [P3]
SetNativeStructFieldsposition contract → Clarified and now proven byTestNativeEmbeddedFieldPositionsacross plain/pointer/qualified/generic embeds. - [P3] README lane wording → Reworded accurately.
New commit c3b9a21d1 (shared aggregate path): Verified correct — comparing against value.Type() in wrapStructConstantAs/wrapStructField skips the storage conversion only on an exact physical-type match, fixing a latent native-narrowed-field bug (e.g. wasm32 i64 uintptr value into an i32 slot) without regressing the common tuple, array, or wide-pointer paths. Well-covered by the new TestWasm32NativeStructAndGCRootStorage assertions.
Security & docs: No issues. The atomic latch is a safe monotonic flag with the mutex still guarding all shared-map access; the cVectorArray panics remain internal-invariant assertions unreachable from valid cgo input (and are now exercised by TestCVectorRejectsIncompatibleGoStorage).
One non-blocking latent item remains (inline). Overall this is a clean, well-tested revision.
| if abi.IsClosureFields(fields) { | ||
| return []int64{0, ptrSize} | ||
| } | ||
| if _, offsets, ok := p.vectorStructLayout(fields, prog.nativeFields(fields)); ok { |
There was a problem hiding this comment.
[P3] Offsetsof derives native-ness from a different source than Sizeof/Alignof, and evaluates nativeFields outside the fast-path guard.
Two related observations on this line:
-
Latent desync (one-shot API only).
Offsetsofcomputes native-ness fromprog.nativeFields(fields)(keyed onfields[0].Pos()), whereasSizeof/Alignofcompute it from the enclosing*types.Named's call conv viahasNativeStructLayout. For a//llgo:cdeclstruct that (a) contains a vector-bearing field — sovectorStructLayoutreturnsok=true— and (b) also contains a function field or trailing zero-sized field, the two paths diverge when the struct was registered viaSetTypeCallConvwithoutSetNativeStructFields:Offsetsofwould usenative=false(func field = two words, trailing zero-field +1) whileSizeof/Alignofusenative=true, so offsets stop agreeing with size. The normal cgo path is safe —handleTypeDecl(cl/import.go:1132-1143) always calls both setters together — so this is reachable only through the one-shot API.TestCVectorByteConstantsAndLayoutBoundsregisters exactly such aCallbackstruct viaSetTypeCallConvalone and assertsSizeof == 32, but never callsOffsetsofon it; adding that assertion would surface the divergence. Worth either derivingOffsetsofnative-ness from the same source asSizeof, or documenting the invariant explicitly. -
Residual guard gap (minor perf).
prog.nativeFields(fields)is evaluated as an argument beforevectorStructLayout's internalhasVectorLayoutsguard runs, so it takes anRLock+ map lookup on everyOffsetsofcall even when no vectors/native fields are registered.Sizeof/Alignofshort-circuit on the atomic flag with zero extra work; gatingnativeFieldsthe same way would make the common path symmetric. (AllocsPerRun-based regression test can't catch this — it's a lock, not an allocation.)
1b65c4b to
4ba3ce6
Compare
|
@fennoai /review Please re-review the latest head The preceding head had one failed check: Windows MSVC AMD64 core tests, where TestRuntimeNumGoroutineIncludesNewProc observed before=3 and during=3 instead of 4. Its os/exec child still initialized all of test/go, including runtimeinitgc's goroutine; channel completion does not guarantee that goroutine has exited runtime accounting. A controlled local probe demonstrated 3 -> 4 -> 3 when another goroutine exits. This is evidence for the test's unstable global-count assumption, not a native Windows reproduction (300 original-test process runs on macOS passed). The assertion now lives in its own test package, retaining the exact +1 requirement and native subprocess isolation. Wasm worker and libffi invocations include the independent package, preserving their coverage. No runtime counter or timeout changes. The rebase retained both the merged integer-resize documentation and this PR's C-vector documentation; the duplicate LLDB harness commit dropped because it is already on main. Validation: 300 isolated LLGo process runs, 20 LLGo runs with coverage enabled, 20 official Go runs, the Emscripten two-worker counter test, focused SSA/compiler/build C-vector regressions, native O2 C interop, shell syntax and diff checks passed. Fresh CI is queued; Windows confirmation remains pending. The previous non-blocking one-shot Offsetsof API observation is unchanged by this CI-focused update. |
Re-review at
|
There was a problem hiding this comment.
Re-review at 4ba3ce6 — rebase + CI test-isolation fix verified
The rebase cleaned the branch to exactly four commits with no unrelated base-main content in the locked range (577d51f...4ba3ce6). I confirmed the three previously-reviewed commits (C-vector support, native-field-width aggregate fix, vector-layout-traversal guard) are byte-for-byte unchanged — verified per-file — so all prior conclusions carry over and the only genuinely new content is the test-isolation commit 4ba3ce6.
Test-isolation fix (test: isolate goroutine count from package initialization) — verified correct and complete:
TestRuntimeNumGoroutineIncludesNewProcmoved fromtest/go/(package gotest) to its owntest/go/numgoroutine/(package numgoroutine_test); the old path is removed (no stale copy). The assertion is byte-for-byte identical — same exactbefore+1requirement and sameos/execsubprocess isolation gated on non-wasm.- The root cause is addressed: a dedicated package yields a minimal test binary whose init footprint no longer includes
test/go's fixtures (theruntimeinitgcgoroutine that could still be exiting while the baseline was sampled, giving the Windows MSVCbefore=3/during=3). - Native coverage is retained, not dropped. The Windows MSVC core suite's main batch (
.github/workflows/go.yml) runsgo list ./...through an anchoredgrep -v '^…/test/go$', which excludes only the exacttest/gopackage and still includestest/go/numgoroutine— I confirmed this with the toolchain. So the moved test continues to run on Windows MSVC AMD64, now in its own isolated binary. dev/test_wasm_workers.shis coherent: the now-unnecessary-skipis dropped from the combinedtest/gorun, the dedicated run and the libffi run point attest/go/numgoroutine, preserving wasm-worker and libffi coverage.- Local validation (standard Go): the package vets, compiles, and passes 20/20 runs; the parent +
os/execchild path works.
Standing item (unchanged, non-blocking): the one-shot API observation from the previous review — Offsetsof (ssa/type.go:111) deriving native-ness from nativeFields (position-keyed) rather than the enclosing named type, with nativeFields also evaluated outside the hasVectorLayouts fast-path guard — still applies verbatim, since those files are identical to the prior revision. Not reachable from the normal cgo path; no action required for this CI-focused update.
No new issues. This revision is clean; the fix correctly targets the Windows flakiness without weakening the assertion or its coverage. CI/Windows confirmation on the fresh run remains the author's pending external check (I did not inspect CI logs).
1 participant
C vector typedefs imported through
import "C"currently lose their vector identity when cmd/cgo spells them as Go arrays. A four-lane C addition returns a first lane of 3 instead of 11. Native SIMD structs also disagree with sourceunsafelayout, and ARM64 homogeneous vector aggregates are passed indirectly instead of using the HVA ABI.Recover named 128-bit C integer/float/double vectors from the target Clang AST before type checking, retaining native vector values/storage while preserving Go array operations. Match source layout to native vector fields in imported structs and
//llgo:cdeclstructs, including embedded fields. Extend ARM64 homogeneous aggregate classification to short vectors and preserve whole-argument spilling. Ordinary Go arrays andarchsimdmemory layout remain unchanged.Add executable C import/direct-binding tests for SSE/NEON operations, integer and double vectors, callbacks and exports, native aggregates, pointer/global/heap layout, array conversions, and register exhaustion. Include the suite in native SIMD CI. Diagnose unsupported vector widths, lane types, alignment, and the inspected unsupported record forms; wider vectors, vectorcall, SIMD reflection calls, and Wasm C interop execution remain outside this change.
Validation:
ssa,internal/cabi, andclpackage tests pass. Fullinternal/buildtests pass withGOWORKunset and a 30-minute package timeout; focused Clang vector/type/layout tests pass on Darwin and Linux.cpu.all=off. These include SSE/NEON intrinsics, vector and aggregate callbacks, exports, embedded fields, and register exhaustion.Hosted CI is still queued/running; the results above are local validation.