explicit tail calls: support indirect arguments#151143
explicit tail calls: support indirect arguments#151143folkertdev wants to merge 2 commits intorust-lang:mainfrom
Conversation
8a1cd78 to
6f7eede
Compare
| PassMode::Indirect { on_stack: true, .. } => { | ||
| // FIXME: some LLVM backends (notably x86) do not correctly pass byval | ||
| // arguments to tail calls (as of LLVM 21). See also: | ||
| // | ||
| // - https://github.com/rust-lang/rust/pull/144232#discussion_r2218543841 | ||
| // - https://github.com/rust-lang/rust/issues/144855 | ||
| span_bug!( | ||
| fn_span, | ||
| "arguments using PassMode::Indirect {{ on_stack: true, .. }} are currently not supported for tail calls" | ||
| ) |
There was a problem hiding this comment.
I've kept the span_bug! for now.
It looks like a fix for x86 might get cherry-picked into LLVM 22. If so, I think that is enough support to allow this variant too. At that point riscv would be the next most commonly used target that would miscompile.
This comment has been minimized.
This comment has been minimized.
6f7eede to
a904f16
Compare
This comment has been minimized.
This comment has been minimized.
a904f16 to
fc79542
Compare
This comment has been minimized.
This comment has been minimized.
029786e to
414b969
Compare
|
@bors try=x86_64-msvc-1,x86_64-msvc-2 |
|
Unknown command "try". Run |
|
@bors try job=x86_64-msvc-1,x86_64-msvc-2 |
This comment has been minimized.
This comment has been minimized.
explicit tail calls: support indirect arguments try-job: x86_64-msvc-1 try-job: x86_64-msvc-2
This comment has been minimized.
This comment has been minimized.
|
💔 Test for e8cb9bb failed: CI. Failed job:
|
414b969 to
33942c6
Compare
|
@bors try job=x86_64-msvc-1,x86_64-msvc-2 |
This comment has been minimized.
This comment has been minimized.
explicit tail calls: support indirect arguments try-job: x86_64-msvc-1 try-job: x86_64-msvc-2
33942c6 to
2e004e1
Compare
|
This seems to work, though we should probably run a bunch of try jobs once the code looks good. |
|
|
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
| } | ||
| LocalRef::Operand(arg) => { | ||
| bx.typed_place_copy( | ||
| arg.val.deref(fn_abi.args[i].layout.align.abi), |
There was a problem hiding this comment.
This shouldn't be a deref. arg.val is not a pointer to the argument value, but an operand representing the value itself. Rather I think it is always a OperandValue::Ref(place) where place is the location that the argument is stored at. arg.val.deref() would incorrectly treat the argument as a pointer itself. Would you mind adding a test for tail calling with a u128 argument on Windows?
There was a problem hiding this comment.
I added
assert_eq!(swapper(i128::MIN, i128::MAX), (i128::MAX, i128::MIN));
below, which succeeded earlier. But I'll add a minicore/codegen test for this then, just to be sure.
There was a problem hiding this comment.
I missed the extern "C" bit. The deref did seem to generate the expected code, but I suspect matching on Ref will be more helpful in catching bugs, so I'm using that now.
| // win-NEXT: %0 = load i128, ptr %b | ||
| // win-NEXT: %1 = load i128, ptr %a | ||
| // win-NEXT: store i128 %0, ptr %a | ||
| // win-NEXT: store i128 %1, ptr %b |
There was a problem hiding this comment.
this swapping is a little unfortunate of course. I suspect this is a very rare pattern in practice though, so we just need a correct implementation, not necessarily the most optimal one from the get-go.
tracking issue: #112788
After discussion in #144855, I was wrong and it is actually possible to support tail calls with
PassMode::Indirect { on_stack: false, .. }arguments.Normally an indirect argument with
on_stack: falsewould be passed as a pointer into the caller's stack frame. For tail calls, that would be unsound, because the caller's stack frame is overwritten by the callee's stack frame.Therefore we store the argument for the callee in the corresponding caller's slot. Because guaranteed tail calls demand that the caller's signature matches the callee's, the corresponding slot has the correct type.
To handle cases like the one below, the tail call arguments must first be copied to a temporary, and can only then be copied to the caller's argument slots.
I'm not really familiar with MIR and what tricks/helpers we have, so I'm just cobbling this together. Hopefully we can arrive at something more elegant.