Adds alignment attributes to indirect args and sret slots - #7382
Adds alignment attributes to indirect args and sret slots#7382corleypc wants to merge 11 commits into
Conversation
|
Regarding the possible actions:
|
|
Pushed new commit 2bfcb04 which fixes two more alignment bugs. One is the <=64 bytes store path ignoring the is_packed custom metadata. package main
Packed :: struct #packed {
_: u8,
v: #simd[4]f32,
}
@(export) g: Packed
f :: proc(p: ^Packed, x: #simd[4]f32) {
y := p.v // load is fine, GEP has packed metadata -> load emits align 1
_ = y
p.v = x // segfault here, same GEP, store emitter ignores the metadata
}
main :: proc() { f(&g, {1, 2, 3, 4}) }The other is GEPs on packed fields on globals folding (so nothing to attach the metadata in the first place) on both loads and stores. package main
Packed :: struct #packed {
_: u8,
v: #simd[4]f32,
}
@(export) g: Packed
main :: proc() {
y := g.v // segfault, GEP folds, type align 16 assumed by LLVM
g.v = y // same
}All three segfault on x64, default target. Fixed using the new lb_try_get_alignment (global GEPs transparent there and alignment recovered). Added regression tests to internal. Edit: Oh, well, I guess I'll be looking at darwin IR. :) |
| testing.expect(t, p2.n == -1) | ||
|
|
||
| p2.v = {} // 16 bytes is memset on most targets | ||
| p2.v = {} // zero vector store -> lb_mem_zero_ptr's direct store path on most targets |
There was a problem hiding this comment.
memset was a wrong assumption.
|
While playing with vet variants, looking at &packed.field uses in the library, I found another bug. GEPs derived from a #packed field GEP don't have the is_packed metadata and resolve to type alignment. package main
import "core:simd"
import "core:fmt"
Packed :: struct #packed {
_: u8,
arr: [2]#simd[4]f32, // arr at offs 1, elems at 1 and 17
}
@(export) g: Packed
read_elem :: proc(p: ^Packed, i: int) -> #simd[4]f32 {
return p.arr[i]
}
main :: proc() {
g.arr[0] = {1, 2, 3, 4}
y := #force_no_inline read_elem(&g, 0) // boom
fmt.println(simd.to_array(y))
}Fixed in ed08cda by going back the GEP chain until metadata found or non-GEP reached. (The actual use in the library (nanovg) was fine (well, at least on x86) due to it using f32's throughout, alignment resolving to 4 bytes and the vectorizer merging to unaligned vector stores.) |
…gn structs and array elem type conversions for #packed array fields
|
@gingerBill It is a bit more relaxed than the gcc/clang warning. In particular, if the taken underaligned address is immediately cast to a pointer type with sufficient alignment, there is no err. This includes casts to rawptr, ^u8, ^runtime.Unaligned (see below), basically anything with data alignment of 1 for #packed fields or with alignment N for #max_field_align(N) fields. runtime.Unaligned // A view of `T` at alignment 1. `^Unaligned(T)` may point at unaligned
// memory, e.g. the address of a field of a `#packed` struct, or of a struct
// whose `#max_field_align` is lower than `T`'s alignment.
// Accesses through `.value` are misalignment safe.
Unaligned :: struct($T: typeid) #packed { value: T }This single field #packed wrapper allows explicit downgrade of the claimed alignment of the address, while not erasing its type. It is zero cost (in the sense that all its work is done at compile time). It composes well. Examples:
// f keeps the type knowledge, accesses are safe
f :: proc(u: ^runtime.Unaligned(i64)) { u.value += 1 }
f((^runtime.Unaligned(i64))(&p.n))
elems := ([^]runtime.Unaligned(f32))(raw_data(buf[1:]))
sum := elems[0].value + elems[1].value
There is one downside in that you need to use .value. (It can be avoided for concrete struct types by Some examples of what would be catched by the vet and what is legal A few examples from the library first:
The last one is from "vendor:nanovg" __xformToMat3x4 :: proc(m3: ^[12]f32, t: [6]f32) {....}
....
__xformToMat3x4(&frag.scissorMat, invxform)frag is a ^FragUniforms, which is #packed, and scissorMat is a [12]f32 field. So &frag.scissorMat produced a ^[12]f32, which is data alignment 4, while the #packed frag only guarantees align 1. Vet error. This was actually the only ver error in the entire library. I fixed this by returning the result by value instead of in a ptr param. Another possibility would have been to use ^runtime.Unaligned. (But returning by value also happens to compile to smaller and faster code (at -o:speed, x86, default target, LLVM 22.), because in the original by ptr param return, the compiler has to assume the ptr aliases. (#no_alias would have made it equal)) More examples: import "base:runtime"
P :: struct #packed {
b: u8,
v: #simd[4]f32,
n: i64,
arr: [2]f32,
inner: struct { f: f32, g: f32 },
slice: []i64,
ptr: ^struct { x: f32 },
sub: runtime.Unaligned(f32),
}
p: P
M :: struct #max_field_align(4) {
a: u16,
b: u64, // placed at alignment 4
inner: struct { x: i64 }, // i64 inside, container capped to 4
}
m: M
poly :: proc(x: ^$T) {}
vet_errors :: proc() {
a := &p.v // vet err - simd field, claims align 16
b := &p.n // vet err - i64 field, claims align 8
c := &p.arr[1] // vet err - element of an array field
pp := &p
d := &pp.v // vet err - through ^P (auto deref)
e := &p.inner.f // vet err - field of a nested (even though non-packed) struct
poly(&p.n) // vet err - poly param, no align-1
f := &m.b // vet err - align 8 field of a #max_field_align(4) struct
g := &m.inner.x // vet err - i64 in a nested struct, align cap 4
}
take_unal_addr :: proc(u: ^runtime.Unaligned(#simd[4]f32)) {}
// these all pass
passing :: proc() {
a := &p.b // u8 field, data align 1
b := (^u8)(&p.n) // direct cast to ^u8
c := rawptr(&p.v) // conversion to rawptr
take_unal_addr((^runtime.Unaligned(#simd[4]f32))(&p.v)) // runtime.Unaligned cast
poly((^runtime.Unaligned(i64))(&p.n)) // same, into a poly param, T infers as runtime.Unaligned(i64)
not_packed: struct { f: f32 }
d := ¬_packed.f // not a #packed
e := &p // whole packed struct, data align 1
f := &p.slice[0] // slice element, not packed storage (only slice header in packed)
g := &p.ptr.x // through a pointer field, ptr is in packed but not what it points to
h := &p.sub // pointee itself #packed: align 1
i := &m.a // align 2 field within align 4 cap
j := (^u32)(&m.b) // cast within the align 4 cap
k := (^runtime.Unaligned(u64))(&m.b) // runtime.Unaligned also covers #max_field_align fields
}I haven't added the flag to -vet, so currently it needs to be added explicitly on the command line. Two more segfault bugs fixed along the way, derived GEPs for fields of #max_field_align structs and array element type conversions for array fields in #packed structs. Tests added for these two. If the vet flag is satisfactory, I believe this PR is now complete. |
|
Meanwhile, a few more alignment bugs fixed in 429ee55
This accounts for a big chunk of the change, cause union variant accesses needed fixes per site. For structs, the fix uses existing max-align custom metadata. lb_emit_struct_ep now folds custom_align into the max-align cap (honoring both, if present), so a field GEP carries max-align metadata and lb_adjust_access_alignment_from_addr limits every load/store alignment through it. Raw union fields also get the metadata, cause lb_emit_deep_field_gep now routes them through lb_emit_struct_ep (a zero offset GEP) instead of a pointer cast, and the metadata attaches to the GEP instruction. For unions, the variant pointer is a cast, not a GEP, so the metadata can't attach. Instead, the alignment cap is applied at each site. This needed a lb_emit_store version with max alignment limit, so variant stores go through lb_emit_store_with_max_align(..., type_align_of(union)), variant loads in lb_emit_union_cast are capped with lb_cap_access_alignment. The union tag GEP does get max-align metadata. By-ref type switch case variable is bound to a zero offset GEP (gets the metadata), and for zeroing the capped alignment goes into lb_mem_zero_ptr (its scalar path now honors it). Test cases for all of these added to tests/internal/test_custom_align.odin A minimal segfault example (Linux, x64, LLVM 22) package main
import "core:fmt"
import "core:mem"
S :: struct #align(1) {
a: #simd[4]f32 // type align 16
} // size_of(S) == 16, align_of(S) == 1
// export so it isn't folded
@(export) rs :: proc(s: ^S) -> #simd[4]f32 { return s.a }
main :: proc() {
raw, _ := mem.alloc_bytes(size_of(S), align_of(S)) // align 1 requested, so legal
s := cast(^S)(&raw[0])
fmt.println(rs(s)) // segfault
}
package main
import "core:fmt"
Inner :: struct #min_field_align(16) { v: #simd[4]f32 }
Outer :: struct #packed { x: u8, inner: Inner }
Helper :: struct {
force: #simd[4]f32, // foces align 16
_: u8,
o: Outer, // offset 17, inner.v at 18
}
@(export) h: Helper
@(export)
f :: proc(o: ^Outer) -> #simd[4]f32 {
return o.inner.v
}
main :: proc() {
h.o.inner.v = {1, 2, 3, 4} // const GEP works
fmt.println(#force_no_inline f(&h.o)) // segfault
}Fixed in lb_adjust_access_alignment_from_addr by making #packed have the final say. Test added.
This needs to be built with -o:speed for the segfault to manifest. (Observed on Linux, x64, default target (need SSE for the crash), o:speed, LLVM 22) package main
import "core:fmt"
S :: struct { p: ^[8]u8 }
@(export) backing: [32]u8
@(export) buf: [2]#simd[4]f32
@(export)
f :: proc(s: ^S, t: ^S) -> ([8]u8, #simd[4]f32) {
// s.p^ + t.p^ makes the compiler assume align 8 on the
// loads through s.p/t.p (incorrectly, from the loads' own alignment)
r := s.p^ + t.p^
low3bits := uintptr(rawptr(s.p)) & 7 // optimizer folds the & 7 to 0, cheerfully assuming s.p is align 8
q := (^#simd[4]f32)(rawptr(uintptr(&buf) + 15 + low3bits)) // movaps to buf+15 -> segfault
return r, q^
}
main :: proc() {
// explicitly misalign off to (base+off) & 7 == 1
base := uintptr(&backing[0])
off := int((1 + 8 - (base & 7)) & 7)
for i in 0..<8 {
backing[off+i] = u8(i + 1)
backing[off+8+i] = u8(10 * (i + 1))
}
buf[1] = {1, 2, 3, 4}
s := S{(^[8]u8)(rawptr(&backing[off]))}
t := S{(^[8]u8)(rawptr(&backing[off+8]))}
r, v := #force_no_inline f(&s, &t)
fmt.println(r, v)
}Internal tests are not run at o:speed, so no test added for this one.
Since vectors can appear at tons of store places, fixing this needed adding the OdinLLVMBuildStore and OdinLLVMBuildStoreAligned wrappers (similar already existed for loads so trivial fix there), which honor the alignment. And then replacing stores throughout accounts for a lot of the commit. This was IR and asm verified. -target:linux_arm64 (max_simd_align == 16) V :: #simd[32]u8 // align_of(V) == 16
@(export) loc :: proc(v: V) -> V { x := v; return x }IR: -target:darwin_amd64 -microarch:x86-64-v3 (AVX) (max_simd_align == 16) V :: #simd[8]f32
@(export) ld :: proc(p: ^V) -> V { return p^ }
@(export) al :: proc() -> int { return align_of(V) }ASM: _ld: vmovaps (%rdi), %ymm0 ; retq # requires 32-byte alignment -> would segfault
_al: movl $16, %eax ; retq # align_of(V) only grants align 16 |
(Update: This PR now fixes more than a dozen alignment bugs (read thread).)
The PR adds the align attribute (where align > 1) to the compiler owned indirect arguments for the Odin and contextless calling conventions and on the sret slot for all calling conventions. It also fixes two alignment related bugs (detailed below).
An immediate followup to this (and the main goal of doing this in the first place) is adding the nonnull and dereferenceable attributes, which I am working on and it is shaping as a way smaller change.
A lot of the necessary plumbing came with #7327 by @kalsprite, which I was looking forward to it getting merged in order to do this properly. It landed attribute propagation at call sites, type_align_of(source type) for byval arguments and I used the ABI harness for validation.
What's implemented in the PR
lb_try_get_alignment is now expanded and tries hard to figure "what alignment can be honestly claimed for this LLVM pointer"?
It starts assuming the default storage alignment of T for ^T, then looks for provable
knowledge to override this assumption. Allocas, globals, upstream argument align attributes, phi/selects, chained GEP arithmetic (up to a preconfigered chain depth, currently set to 8) can all override the default alignment (up or down). For example, a pointer to a field of a #packed struct can be resolved below the type default alignment, or GEP arithmetic may upgrade the alignment.
A pass over the arguments (lb_abi_add_indirect_source_type_alignments) runs after the target ABI argument types are known and stamps align(N) attributes (when N>1) on:
The alignment of indirect args is then honored at call sites in lb_emit_call. Alignment of constants is bumped up if insufficient, so that they materialize properly aligned. For non-constant lvalues where lb_try_get_alignment can't prove sufficient alignment (e.g. a #packed field), the value is copied to aligned temp first.
Memory intrinsics (memcpy/memmove/memset) currently claim pessimistic alignment. With the PR, when doing copies each pointer operand goes through lb_mem_copy_ptr_alignment (default = pointed type alignment, possibly overridden by lb_try_get_alignment) before converting to rawptr, and memset gets its alignment passed directly. The unaligned builtins (mem_copy/mem_copy_non_overlapping on rawptrs, unaligned_load/unaligned_store, mem_zero) strip the type or pass align 1, but lb_try_get_alignment can still upgrade them.
A couple of bugs fixed
lb_try_get_alignment currently returns an access alignment (LoadInst) as if it is the loaded pointer value alignment which fed lb_emit_store memmove. The PR treats loads as opaque.
Call emits currently pass &packed.field as an indirect arg while the callee assumes default alignment which could miscompile on alignment strict targets (RiscV64, maybe Arm32) and can segfault on x86 (replication of the segfault in the PR added regression test). The new alignment check with potential aligned temp copy prevents this.
What does this get us?
Alignment knowledge that can't always be re-derived b the optimizer now flows. As mentioned, the real goal is having nonnull and deferenceable in, but the alignment attribute nets positives on its own too. On strict alignment targets (like riscv64), align 1 memove forces a libcall, while align 8 leads to inline load/stores (verified in IR).
A before/after example on x86
Built with -o:speed, default arch target (SSE, but no AVX), LLVM 22.
ASM before
Code assumes align 1 and must load every vector into a register using unaligned load before the addition.
ASM after
Code knows alignment is 16 and can add directly from aligned memory.
The copy (technically memmove) is elided in both cases, but in the current version LLVM must assume align 1, cause the local copy loses alignment provenance. (Struct size must be > 64 for this to show up, at <= 64 the stores are built directly in the backend.)
Also note that AVX is not sensitive to alignment, so this won't show up in AVX targeting code.
Adding the dereferenceable attribute (coming soon!) would allow the optimizer to do a bunch of neat tricks with load speculation like hoisting loads out of loops, vectorizing conditional loads, flattening if/elses's, and more. And alignment is prerequisite for dereferenceable, cause the optimizer only speculates with known (and sufficient) alignment.
We need to talk about addresses of packed fields
I believe everything knowable by analysis is addressed, but there are language constructs that actively put a stick in the wheel. (These aren't PR introduced but relate to alignment, so a proper time for them to get a mention).
p: ^Big = &packed.bigfield; f(p^)the address reaches the call site from a load, and a loaded ^T is trusted as align_of(T) by convention. Alignment analysis can't see through a store/load pair.
1 and 2 appear for #packed structs or when #max_field_align is less than the field T alignment. If we want to address them, it is a language level thing. In C, dereferencing a pointer to a packed field is UB and clang/gcc have warnings on by default for taking the address of a packed field. In Go, there are no packed structs at all and packing goes through encoding/binary. In Rust, taking the address of a packed field is a compilation error.
Possible actions we can take for Odin.
Document explicitly in the spec (and overview) what the backend relies on. That is, a ^T is presumed align_of(T); underaligned access goes through rawptr or unaligned intrinsics. Basically, define p^ on an underaligned storage as a a programmer bug.
Vet warning like gcc/clang. This can be placed on taking the address or at use, or both. Conversions to rawptr (maybe byte multipoinmters?) can be exempt. This should be cheap to implement, cause field offsets and alignments are static, and the checker has everything it needs.
Runtime checks in debug builds. Emits are essentially the assertions to check, so instrumentation is small addition to emission time, not some new analysis. (Will also catch some of the unsafe stuff in 3.)
Just hard error on &packed.field as ^T, like Rust. :) Will break existing code (if any),
Suggestions?