Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix fn_sig_for_fn_abi and the coroutine transform for generators #118219

Merged
merged 3 commits into from
Nov 24, 2023
Merged

Fix fn_sig_for_fn_abi and the coroutine transform for generators #118219

merged 3 commits into from
Nov 24, 2023

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Nov 23, 2023

There were three issues previously:

  • The self argument was pinned, despite Iterator::next taking an unpinned mutable reference.
  • A resume argument was passed, despite Iterator::next not having one.
  • The return value was CoroutineState<Item, ()> rather than Option

While these things just so happened to work with the LLVM backend, cg_clif does much stricter checks when trying to assign a value to a place. In addition it can't handle the mismatch between the amount of arguments specified by the FnAbi and the FnSig.

@rustbot
Copy link
Collaborator

rustbot commented Nov 23, 2023

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added 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 Nov 23, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 23, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@bjorn3
Copy link
Member Author

bjorn3 commented Nov 23, 2023

Fixes #116447 (comment)

@bjorn3 bjorn3 mentioned this pull request Nov 23, 2023
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Fix fn_sig_for_fn_abi and the coroutine transform for generators
b7bc8d5 
There were three issues previously:
* The self argument was pinned, despite Iterator::next taking an
  unpinned mutable reference.
* A resume argument was passed, despite Iterator::next not having one.
* The return value was CoroutineState<Item, ()> rather than Option<Item>

While these things just so happened to work with the LLVM backend,
cg_clif does much stricter checks when trying to assign a value to a
place. In addition it can't handle the mismatch between the amount of
arguments specified by the FnAbi and the FnSig.
@rustbot rustbot added the A-meta Area: Issues about the rust-lang/rust repository. label Nov 23, 2023
compiler-errors
compiler-errors approved these changes Nov 23, 2023
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

r=me with or without doing more exhaustive matching on coroutine kind

compiler/rustc_ty_utils/src/abi.rs Outdated Show resolved Hide resolved
compiler/rustc_ty_utils/src/abi.rs Outdated Show resolved Hide resolved
@compiler-errors
Copy link
Member

Thanks @bjorn3 for catching this. I really appreciate that clif is a lot less forgiving with ABI -- you've already caught a few of these mismatched in the past.

@bjorn3
Copy link
Member Author

bjorn3 commented Nov 23, 2023

Added exhaustive matching and fixed the assertion on the resume type.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member Author

bjorn3 commented Nov 23, 2023

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Nov 23, 2023

📌 Commit 543e559 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 23, 2023
@bors
Copy link
Contributor

bors commented Nov 24, 2023

⌛ Testing commit 543e559 with merge 1fd418f...

@bors
Copy link
Contributor

bors commented Nov 24, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 1fd418f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 24, 2023
@bors bors merged commit 1fd418f into rust-lang:master Nov 24, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Nov 24, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1fd418f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This 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.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.904s -> 675.686s (-0.03%)
Artifact size: 313.71 MiB -> 313.72 MiB (0.00%)

@bjorn3 bjorn3 deleted the fix_generator_fn_abi branch November 24, 2023 09:05
@dlight
Copy link

dlight commented Jan 14, 2024

While these things just so happened to work with the LLVM backend, cg_clif does much stricter checks when trying to assign a value to a place.

Here you did mention that despite the LLVM backend accepting it, it was triggering UB. Does this PR fixes this for LLVM as well?

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 14, 2024

Yes

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

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees

@compiler-errors compiler-errors

Labels
A-meta Area: Issues about the rust-lang/rust repository. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.
8 participants
@bjorn3 @rustbot @rust-log-analyzer @compiler-errors @bors @rust-timer @dlight @davidtwco