Emit @llvm.lifetime.end for moved arguments passed indirectly
#98121
Conversation
This results in code like:
```rust
fn array_dead_store(mut x: [u8; 1234]) {
x[0] = 42;
}
```
Getting compiled to:
```ll
define void @array_dead_store([1234 x i8]* %x) {
%0 = getelementptr inbounds [1234 x i8], [1234 x i8]* %x, i64 0, i64 0
store i8 42, i8* %0 ; this store can be dropped
tail call void @llvm.lifetime.end.p0i8(i64 1234, i8* nonnull %0)
ret void
}
```
...which allows LLVM to drop the dead store.
Without the lifetime marker, `%x` is just a pointer argument, so LLVM
does not know that it is unused after the function returns.
rustbot
added
the
T-compiler
rust-highfive
added
the
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Hmm, not reproducible locally (with LLVM 14), wonder if it's a miscompile @rustbot author |
rustbot
added
S-waiting-on-author
There was a problem hiding this comment.
Ideally this would be solved with an additional attribute on the LLVM side, because inserting lifetime.end only covers the normal return paths, but not the unwind paths. But adding lifetime.end does seem like a reasonable intermediate solution.
Those test failures are quite concerning...
| @@ -58,6 +60,8 @@ pub fn array_eq_zero_mid(x: [u16; 8]) -> bool { | |||
| // CHECK-NEXT: start: | |||
| // CHECK: %[[LOAD:.+]] = load i128, | |||
| // CHECK-NEXT: %[[EQ:.+]] = icmp eq i128 %[[LOAD]], 0 | |||
| // CHECK-NEXT: bitcast | |||
There was a problem hiding this comment.
Drop bitcast here and use CHECK for the lifetime. Similar for other bitcast check lines. Otherwise these tests will fail on LLVM 15 with opaque pointers.
| } | ||
| match self.locals[local] { | ||
| LocalRef::Place(place) => place.storage_dead(&mut bx), | ||
| LocalRef::UnsizedPlace(place) => place.storage_dead(&mut bx), |
There was a problem hiding this comment.
Is there a test for the unsized case?
| continue; | ||
| } | ||
| let abi = &self.fn_abi.args[arg_index]; | ||
| if !abi.is_indirect() { |
There was a problem hiding this comment.
I wonder whether we should special-case byval indirect here? Adding the lifetime marker isn't wrong in that case, just not necessary.
|
To double check, for @bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit eb05884 with merge 1ecc9861782cbff553587957c3d2f3f5f27097c6... |
|
After inlining the llvm.lifetime.end intrinsic will have much stronger unintended semantics. When applied to a stack object it will incorrectly shorten the object's lifetime. And in the case where stack object didn't use any lifetime intrinsics, adding llvm.lifetime.end will make object dead everywhere. |
|
☀️ Try build successful - checks-actions |
|
Queued 1ecc9861782cbff553587957c3d2f3f5f27097c6 with parent ddb6cc8, future comparison URL. |
From a cursory look, I believe we do emit lifetime.start for these stack objects. We'll have two lifetime.end intrinsics afterwards, which is weird, but not illegal. (Though maybe the lifetime.start emission isn't reliable -- you are definitely right that we should not emit lifetime.end if there is no lifetime.start before the call.) |
There is a number of situations under which we don't use lifetime intrisics. To give one example: calls using const operands This also brings more fundamental question of MIR semantics. In call terminators we pass |
|
Finished benchmarking commit (1ecc9861782cbff553587957c3d2f3f5f27097c6): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Footnotes |
rustbot
added
perf-regression
Yes: https://godbolt.org/z/Yz8en8684
It wasn't clear to me from the langref if If it is the case that this is only safe if (at least) there is a Potentially, we could also (ab)use
Huh. Independently of this change, seems like we should be using lifetime intrinsics there? I guess the reason we don't currently is that there's nowhere to put (Yep, passing const arguments directly wastes stack space like you'd expect: https://godbolt.org/z/Kn7b7nTnd) Edit: opened #98156 for this
Yeah, this seems like a problem. Again (ab)using |
Ah, the reference was updated last year to clarify that "If the returned pointer is used by llvm.lifetime.start the returned object is initially dead". In that case it would be fine. The stack coloring itself considers a slot "interesting" if it has any sort of lifetime marker (either start or end) with slots being initially dead. Though, slots with empty live ranges are not merged, so this might be consistent with the reference.
It would seem that also adding lifetime.start would solve one problem but create another in the case there are no markers. |
Yeah, makes sense. I don't think this approach is viable then |
This results in code like:
Getting compiled to:
...which allows LLVM to drop the dead store.
Without the lifetime marker,
%xis just a pointer argument, so LLVMdoes not know that it cannot be used after the function returns.
Fixes (partially) #96497
r? @nikic