Skip to content

explicit tail calls: pass caller's argument slots as arguments for untuple indirect arguments#158248

Open
dianqk wants to merge 1 commit into
rust-lang:mainfrom
dianqk:tail-untuple-indirect
Open

explicit tail calls: pass caller's argument slots as arguments for untuple indirect arguments#158248
dianqk wants to merge 1 commit into
rust-lang:mainfrom
dianqk:tail-untuple-indirect

Conversation

@dianqk

@dianqk dianqk commented Jun 22, 2026

Copy link
Copy Markdown
Member

tracking issue: #112788

Fixes #158017.

Same as #151143, but for untuple indirect arguments.

r? WaffleLapkin

@rustbot rustbot added F-explicit_tail_calls `#![feature(explicit_tail_calls)]` S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 22, 2026
@rustbot

rustbot commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

WaffleLapkin is not on the review rotation at the moment.
They may take a while to respond.

// temporaries, then copy back to the caller's argument slots.
// Finally, we pass the caller's argument slots as arguments.
//
// To do that, the argument must be MUST-by-move value.

@dianqk dianqk Jun 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MUST-by-move is removed in this PR.
FYI @nikic

View changes since the review

Comment on lines -1311 to -1313
let local = self.mir.args_iter().nth(i).unwrap();

match &self.locals[local] {

@dianqk dianqk Jun 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a reliable way to get the LLVM value for the argument. We are allowed to introduce a copy alloca in this case:

let llarg = bx.get_param(llarg_idx);
llarg_idx += 1;
LocalRef::Place(PlaceRef::new_sized(llarg, arg.layout))
.
cc @folkertdev

View changes since the review

Comment thread compiler/rustc_codegen_ssa/src/mir/block.rs Outdated
Comment thread tests/codegen-llvm/tail-call-untuple.rs Outdated
Comment on lines +1331 to +1332
// We don't need `tail_call_temporaries` for the untuple arguments because they won't be
// modified, they are the last arguments.

@WaffleLapkin WaffleLapkin Jun 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get what this comment is saying...

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new comment should be clearer.

@dianqk dianqk force-pushed the tail-untuple-indirect branch from 5493c9b to a61a2b4 Compare June 23, 2026 06:35
Comment on lines +1331 to +1336
// For untupled arguments, it is safe to store them directly into the caller's
// argument slots without temporaries, because the untupled arguments are
// always passed through a tuple alloca. The alloca serves as a temporary
// that does not overlap with any caller argument.
// No temporaries are needed for the caller's untupled arguments either,
// because they are used last.

@WaffleLapkin WaffleLapkin Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't quite get this. First of all, this should probably mention tail calls, as otherwise it doesn't make sense why anything here wouldn't be ok.

But second of all, I checked out your branch locally and I'm getting incorrect codegen (rustc t.rs --emit=llvm-ir -O):

#![feature(unboxed_closures)]
#![feature(explicit_tail_calls)]
#![crate_type = "lib"]

type Tuple = (u64, u64, u64, u64);

trait X {
    extern "rust-call" fn f(self, args: Tuple) -> u8;
    extern "rust-call" fn g(self, args: Tuple) -> u8;
}

impl X for Tuple {
    extern "rust-call" fn f(self, args: Tuple) -> u8 {
        become args.g(self)
    }

    #[inline(never)]
    extern "rust-call" fn g(self, args: Tuple) -> u8 {
        std::hint::black_box((self, args));
        0
    }
}
; <(u64, u64, u64, u64) as t::X>::f
; Function Attrs: nounwind nonlazybind uwtable
define noundef i8 @_RNvXCs146zwHEvxau_1tTyyyyENtB2_1X1f(ptr dead_on_return noalias nofree noundef align 8 captures(none) dereferenceable(32) initializes((0, 32)) %self, i64 noundef %0, i64 noundef %1, i64 noundef %2, i64 noundef %3) unnamed_addr #0 {
start:
  store i64 %0, ptr %self, align 8
  %.sroa.4.0.self.sroa_idx = getelementptr inbounds nuw i8, ptr %self, i64 8
  store i64 %1, ptr %.sroa.4.0.self.sroa_idx, align 8
  %.sroa.5.0.self.sroa_idx = getelementptr inbounds nuw i8, ptr %self, i64 16
  store i64 %2, ptr %.sroa.5.0.self.sroa_idx, align 8
  %.sroa.6.0.self.sroa_idx = getelementptr inbounds nuw i8, ptr %self, i64 24
  store i64 %3, ptr %.sroa.6.0.self.sroa_idx, align 8
; call <(u64, u64, u64, u64) as t::X>::g
  %4 = musttail call noundef i8 @_RNvXCs146zwHEvxau_1tTyyyyENtB2_1X1g(ptr noalias nofree noundef nonnull align 8 captures(address) dereferenceable(32) %self, i64 noundef %0, i64 noundef %1, i64 noundef %2, i64 noundef %3) #4
  ret i8 %4
}

Granted, this code is weird and I couldn't come up with a more normal looking one, but I don't get the justification for why this is sound...

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for the case.

@WaffleLapkin

Copy link
Copy Markdown
Member

I could honestly see us just banning tail calling extern "rust-call" functions. There is really no reason to support that I think.

@dianqk dianqk force-pushed the tail-untuple-indirect branch from a61a2b4 to c48e49c Compare June 23, 2026 13:52
@dianqk

dianqk commented Jun 23, 2026

Copy link
Copy Markdown
Member Author

I could honestly see us just banning tail calling extern "rust-call" functions. There is really no reason to support that I think.

I don’t have a strong opinion on this decision. I think it’s up to you.

@rust-log-analyzer

Copy link
Copy Markdown
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
.........F..................................       (144/144)

======== tests/rustdoc-gui/sidebar-resize-window.goml ========

[ERROR] line 35: TimeoutError: Navigation timeout of 30000 ms exceeded: for command `reload:`
    at <file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/test_docs/index.html>
[ERROR] line 36: ProtocolError: Emulation.setDeviceMetricsOverride timed out. Increase the 'protocolTimeout' setting in launch/connect calls for a higher timeout if needed.: for command `set-window-size: (1280, 600)`
    at <file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/test_docs/index.html>


<= doc-ui tests done: 143 succeeded, 1 failed, 0 filtered out

Error: ()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

F-explicit_tail_calls `#![feature(explicit_tail_calls)]` S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed assert got 1400 instead of 14 after become tail call at -O3

4 participants