Allow BackendRepr::Scalar for repr(Rust, packed) structs [MCP1007]#159033
Allow BackendRepr::Scalar for repr(Rust, packed) structs [MCP1007]#159033scottmcm wants to merge 1 commit into
BackendRepr::Scalar for repr(Rust, packed) structs [MCP1007]#159033Conversation
rustbot
added
S-waiting-on-review
|
|
| pub unsafe fn write_packed_scalar(ptr: *mut PackedScalar, val: PackedScalar) { | ||
| // CHECK: start | ||
| // CHECK-NEXT: store i64 %val, ptr %ptr, align 2 | ||
| // CHECK-NEXT: ret void | ||
| *ptr = val; | ||
| } |
There was a problem hiding this comment.
Compare what we emit today: https://rust.godbolt.org/z/8Pv61EhbK
This one, for example, emits 2 allocas and 2 memcpys without this PR:
define void @write_packed_scalar(ptr noundef %ptr, i64 noundef %0) unnamed_addr {
start:
%1 = alloca [8 x i8], align 8
%val = alloca [8 x i8], align 2
call void @llvm.lifetime.start.p0(ptr %1)
store i64 %0, ptr %1, align 8
call void @llvm.memcpy.p0.p0.i64(ptr align 2 %val, ptr align 8 %1, i64 8, i1 false)
call void @llvm.lifetime.end.p0(ptr %1)
call void @llvm.memcpy.p0.p0.i64(ptr align 2 %ptr, ptr align 2 %val, i64 8, i1 false)
ret void
}(which LLVM of course cleans up even in -O1, but still nice to just not emit that in the first place.)
| #[repr(C, packed)] | ||
| #[repr(Rust, packed)] | ||
| struct Packed<T>(T); |
There was a problem hiding this comment.
I'd used C here out of paranoia -- probably from remembering how repr rust unions are useless -- but Rust is fine. The code using this double-checks the properties it cares about with const-asserts too:
rust/library/core/src/ptr/mod.rs
Lines 1812 to 1815 in 3664b37
There was a problem hiding this comment.
What even if this type for? A doc comment would be good.
There was a problem hiding this comment.
Removing this from this PR to do it in #159157 first instead.
| fn check_layout_abi<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLayout<'tcx>) { | ||
| // Verify the ABI-mandated alignment and size for scalars. | ||
| let align = layout.backend_repr.scalar_platform_align(cx); | ||
| // Verify the size expectation for scalars. |
There was a problem hiding this comment.
| // Verify the size expectation for scalars. | |
| // Verify the size expectation for scalars. | |
| // Note that there is no alignment requirement, except for the | |
| // implicit requirement that `align <= scalar_size` because otherwise | |
| // the size check would fail due to the padding. |
| #[repr(C, packed)] | ||
| #[repr(Rust, packed)] | ||
| struct Packed<T>(T); |
There was a problem hiding this comment.
What even if this type for? A doc comment would be good.
scottmcm
added
S-waiting-on-author
…uct, r=joshtriplett
Better comment the struct used by `{read,write}_unaligned`
To address rust-lang#159033 (comment)
r? @RalfJung
|
☔ The latest upstream changes (presumably #159166) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
Rollup merge of #159157 - scottmcm:clearer-ptr-unaligned-struct, r=joshtriplett Better comment the struct used by `{read,write}_unaligned` To address #159033 (comment) r? @RalfJung
4 participants
Tracking issue: #158985
This only changes
Scalar(notScalarPairnorSimdVector) because I wanted to do this in small bites. It also preserves the requirement that there can't be padding, so won't apply foralign(MORE_THAN_SIZE)either. I also didn't touch unions.Note that after #158666 there are zero mentions of
default_alignin codegen, so this "just working" in codegen without any changes is IMHO expected. Codegen has long been expected to always use PlaceValue::align instead of anything from the layout when dealing with alignment.r? workingjubilee