Repository navigation
fix(ssa): honor 32-bit Go alignment for basic values and arrays - #3025
Merged
cpunion merged 4 commits intoNov 10, 2026
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
This is a well-targeted, minimal fix for the alignment mismatch in #2809: on 32-bit Go word targets, a FieldAddr-derived pointer to a 64-bit scalar (e.g. int64/float64, where LLVM's ABI alignment is 8) is only guaranteed 4-byte aligned, so the emitted load/store must not over-claim 8-byte alignment. The helper is placed consistently next to the existing SIMD/wide-pointer alignment handling, runs only on the plain-scalar path (after the vkSIMD and needsWidePointerStorage early returns), and only ever lowers alignment — which is always memory-safe and, if anything, removes a potential misaligned-access fault on strict-alignment 32-bit ARM configs.
Correctness, performance, and security all check out:
- The guard
GoWordSize() != 4cheaply scopes the fix to 32-bit targets, and the 64-bit hot path returns immediately on a cached field read (no added cost on amd64/arm64). AlignOf(typ) > GoWordSize()correctly leaves already-4-aligned scalars untouched and only weakens over-alignment, matching the "weaker alignment is still valid" reasoning.- The
int(...)casts carry small power-of-two values; no truncation/overflow is reachable. - The test matrix is well chosen: it includes both
linux/386(LLVM already 4-aligns, helper is a no-op) andwindows/386(LLVM 8-aligns, helper actively lowers), plus 64-bit controls, and verifies both the direct-pointer andFieldAddrpaths viaVerifyModule+ object emission.
Only two minor, non-blocking clarity nits are inline below. A couple of optional follow-ups: the map iteration over {"load","store"} is nondeterministic but harmless (independent checks, named diagnostics), and on 32-bit targets the per-access AlignOf makes an uncached LLVM ABITypeAlignment FFI call on every scalar Load/Store — well-bounded by the preceding *types.Basic filter, but cacheable (e.g. via the existing hostBasicLayouts table) if 32-bit compile time ever becomes a concern.
No blocking issues found.
| // escape, so accesses must honor the Go pointer guarantee rather than relying | ||
| // on the alignment of the original allocation. Native pointers may be more | ||
| // aligned; the weaker alignment is still valid for those accesses. | ||
| func (b Builder) setGoScalarAccessAlignment(inst llvm.Value, typ Type) { |
Contributor
There was a problem hiding this comment.
Minor naming/scope nit (behavior is correct): the name setGoScalarAccessAlignment and the "scalar type" wording in the comment undersell what the *types.Basic guard actually matches. complex64/complex128 are also *types.Basic, and they lower to LLVM aggregate structs ({float,float} / {double,double}), not scalars — yet the helper does (correctly) fire for complex128 on windows/386, where its ABI alignment is 8 > 4. SetAlignment on an aggregate load/store is valid, so this is fine, but a future reader may wrongly assume complex types are excluded. Consider renaming to e.g. setGoBasicAccessAlignment / setGoValueAccessAlignment, or amending the comment to note it also covers complex basic types.
| // A 32-bit Go scalar pointer may refer to a four-byte-aligned struct field, | ||
| // even when LLVM gives its scalar type eight-byte ABI alignment. FieldAddr can | ||
| // escape, so accesses must honor the Go pointer guarantee rather than relying | ||
| // on the alignment of the original allocation. Native pointers may be more |
Contributor
There was a problem hiding this comment.
Minor doc clarity: "Native pointers may be more aligned" is ambiguous. The *types.Basic guard means this helper never runs on pointer-typed Go values, and "native" collides with the codebase's own isNativeStorage/needsWidePointerStorage concepts. The intended meaning seems to be that the runtime address may happen to be more strongly aligned, and declaring only the Go-guaranteed 4-byte alignment is still correct because a more-aligned address trivially satisfies a weaker requirement. Suggest rephrasing to something like: "The underlying address may in fact be more strongly aligned at runtime; declaring only the Go-guaranteed alignment is still correct because a more-aligned pointer satisfies a weaker alignment requirement."
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 |
zhouguangyuan0718
force-pushed
the
codex/fix-2809-go-access-alignment
branch
from
November 8, 2026 23:45
1fa2faa to
111642a
Compare
LLGo baseline benchmarks
String equality benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
cpunion
reviewed
Nov 9, 2026
| if b.Prog.GoWordSize() != 4 { | ||
| return | ||
| } | ||
| if _, ok := typ.RawType().Underlying().(*types.Basic); ok { |
Collaborator
There was a problem hiding this comment.
Non-blocking follow-up: this fixes the basic-type accesses, while ordinary whole-array accesses still have the same alignment overclaim. For example:
type S struct {
B byte
V [2]int64
}
func read(p *S) [2]int64 { return p.V }
func write(p *S, v [2]int64) { p.V = v }On Windows/386, V is at offset 4 in a struct aligned to 4 bytes, but these functions still emit load [2 x i64] / store [2 x i64] with align 8. The *types.Basic guard excludes this array type; nested arrays have the same issue. This is an invalid alignment guarantee under LLVM's load/store contract.
Verified with LLVM 22 on both the unchanged base a87e123ec and this head beb88875c; the PR head was also checked through ordinary Go source lowering. The same array probe still reproduces on #2937 and #3029. These were cross-target IR checks, not native Windows execution.
This is a pre-existing issue and does not block this PR's scalar fix. Please track whole-array/aggregate access alignment as a follow-up to #2809.
zhouguangyuan0718
changed the title
zhouguangyuan0718
changed the title
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
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.
On 32-bit targets, ordinary Go pointers to basic values and arrays of basic values can have four-byte alignment, while LLVM assigns their storage types eight-byte alignment. This includes pointers escaping from struct fields and whole-array reads/writes of named or nested arrays.
Cap these ordinary load/store alignments at the Go word alignment. Unwrap array element types without changing struct layouts or the storage alignment of native, HostLayout, and SIMD structs. When an i64 load/store becomes atomic, retain its stronger eight-byte alignment contract so Windows/386 does not emit unresolved
__atomic_load/__atomic_storehelpers.Validation passed locally with LLVM 22 on macOS/arm64:
go test ./ssa ./ssa/abi ./cl ./internal/cabi -short -count=1 -timeout=15m.Native Windows execution was not performed locally; hosted CI provides that validation.
Fixes #2809. Also covers the array accesses reported in review.