Use GEP inbounds for ZST and DST field offsets #122048
Conversation
For the former, it's fine for `inbounds` offsets to be one-past-the-end, so it's okay even if the ZST is the last field in the layout: > The base pointer has an in bounds address of an allocated object, > which means that it points into an allocated object, or to its end. https://llvm.org/docs/LangRef.html#getelementptr-instruction For the latter, even DST fields must always be inside the layout (or to its end for ZSTs), so using inbounds is also fine there.
rustbot
added
S-waiting-on-review
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
Use GEP inbounds for ZST and DST field offsets ZST field offsets have been non-`inbounds` since I made [this old layout change](https://github.com/rust-lang/rust/pull/73453/files#diff-160634de1c336f2cf325ff95b312777326f1ab29fec9b9b21d5ee9aae215ecf5). Before that, they would have been `inbounds` due to using `struct_gep`. Using `inbounds` for ZSTs likely doesn't matter for performance, but I'd like to remove the special case. DST field offsets have been non-`inbounds` since the alignment-aware DST field offset computation was first [implemented](erikdesjardins@a2557d4#diff-04fd352da30ca186fe0bb71cc81a503d1eb8a02ca17a3769e1b95981cd20964aR1188) in 1.6 (back then `GEPi()` would be used for `inbounds`), but I don't think there was any reason for it. Split out from rust-lang#121577 / rust-lang#121665. r? `@oli-obk` cc `@RalfJung` -- is there some weird situation where field offsets can't be `inbounds`? Note that it's fine for `inbounds` offsets to be one-past-the-end, so it's okay even if there's a ZST as the last field in the layout: > The base pointer has an in bounds address of an allocated object, which means that it points into an allocated object, or to its end. [(link)](https://llvm.org/docs/LangRef.html#getelementptr-instruction) For rust-lang/unsafe-code-guidelines#93, zero-offset GEP is (now) always `inbounds`: > Note that getelementptr with all-zero indices is always considered to be inbounds, even if the base pointer does not point to an allocated object. [(link)](https://llvm.org/docs/LangRef.html#getelementptr-instruction)
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (54c6dee): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 646.18s -> 644.352s (-0.28%) |
rustbot
removed
the
S-waiting-on-perf
|
This seems fine to me. In fact I was assuming we'd already use |
| extern { | ||
| pub type Extern; | ||
| } | ||
|
|
||
| // CHECK-LABEL: @dst_extern | ||
| #[no_mangle] | ||
| pub fn dst_extern(s: &Dst<Extern>) -> &Extern { | ||
| // Computing the alignment of an extern type is currently unsupported and just panics. | ||
|
|
||
| // CHECK: call void @{{.+}}panic | ||
| &s.z | ||
| } |
There was a problem hiding this comment.
Not sure if this is what you expected...
Full IR:
@str.0 = internal unnamed_addr constant [66 x i8] c"attempted to compute the size or alignment of extern type `Extern`"
define align 1 ptr @dst_extern(ptr align 4 %s) unnamed_addr #0 {
start:
; call core::panicking::panic_nounwind
call void @_ZN4core9panicking14panic_nounwind17h5c216434c597abbdE(ptr align 1 @str.0, i64 66) #2
%_0 = getelementptr inbounds i8, ptr %s, i64 5
ret ptr %_0
}funnily enough it still generates an unreachable GEP.
There was a problem hiding this comment.
yea, as long as we have a test, whatever the behaviour, that is fine with me. I just want to protect against accidentally changing behaviour here in the future
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (79d2461): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 648.175s -> 647.024s (-0.18%) |
ZST field offsets have been non-
inboundssince I made this old layout change. Before that, they would have beeninboundsdue to usingstruct_gep. Usinginboundsfor ZSTs likely doesn't matter for performance, but I'd like to remove the special case.DST field offsets have been non-
inboundssince the alignment-aware DST field offset computation was first implemented in 1.6 (back thenGEPi()would be used forinbounds), but I don't think there was any reason for it.Split out from #121577 / #121665.
r? @oli-obk
cc @RalfJung -- is there some weird situation where field offsets can't be
inbounds?Note that it's fine for
inboundsoffsets to be one-past-the-end, so it's okay even if there's a ZST as the last field in the layout:For rust-lang/unsafe-code-guidelines#93, zero-offset GEP is (now) always
inbounds: