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

Reduce confusion about make_indirect_byval by renaming it #130450

Merged
merged 4 commits into from
Sep 19, 2024
Merged

Reduce confusion about make_indirect_byval by renaming it #130450

merged 4 commits into from
Sep 19, 2024

Conversation

workingjubilee
Copy link
Member

As part of doing so, remove the incorrect handling of the wasm target's make_indirect_byval (i.e. using it at all).

Fixes #130442

r? @bjorn3

Advice requested: what to do about tests/codegen/repr/transparent-struct-ptr.rs? It seems incorrect to simply remove wasm from that one... does another test cover the relevant handling?

@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 Sep 16, 2024
@bjorn3
Copy link
Member

bjorn3 commented Sep 16, 2024

Advice requested: what to do about tests/codegen/repr/transparent-struct-ptr.rs? It seems incorrect to simply remove wasm from that one... does another test cover the relevant handling?

I think adding wasm to other files in that directory as appropriate is what is supposed to happen.

@workingjubilee
Copy link
Member Author

@bjorn3 What confuses me is that the other ABIss in transparent-struct-ptr seem to exhibit the pass-by-hidden-pointer ABI, but they also use the byval attr, so something feels off in our understanding of what byval means.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member

bjorn3 commented Sep 16, 2024

The way LLVM IR and Cranelift IR both work is that for byval you pass a pointer at the IR level, LLVM/Cranelift copies the value it points to to the right location and at the callee side a pointer to the right stack location is materialized out of thin air pretending to be a regular argument. This means the frontend can produce the exact ir (except for the byval attribute) whether large structs are passed using a pointer or at a fixed stack location.

@workingjubilee
Copy link
Member Author

The way LLVM IR and Cranelift IR both work is that for byval you pass a pointer at the IR level, LLVM/Cranelift copies the value it points to to the right location and at the callee side a pointer to the right stack location is materialized out of thin air pretending to be a regular argument. This means the frontend can produce the exact ir (except for the byval attribute) whether large structs are passed using a pointer or at a fixed stack location.

Ah.
...so are we sure the change we want isn't to just skip checking for byval inside transparent-struct-ptr.rs?

@workingjubilee workingjubilee force-pushed the these-names-are-indirect branch from 7e2cad3 to 30be609 Compare September 17, 2024 01:52
@workingjubilee
Copy link
Member Author

...so are we sure the change we want isn't to just skip checking for byval inside transparent-struct-ptr.rs?

After some mulling it over I split out that test for wasm.

@workingjubilee workingjubilee marked this pull request as ready for review September 17, 2024 05:23
@bjorn3
Copy link
Member

bjorn3 commented Sep 17, 2024

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Sep 17, 2024

📌 Commit 30be609 has been approved by bjorn3

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 Sep 17, 2024
@bors
Copy link
Contributor

bors commented Sep 17, 2024

⌛ Testing commit 30be609 with merge 8d3f80b...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2024
@bors
Auto merge of rust-lang#130450 - workingjubilee:these-names-are-indir…
8d3f80b 
…ect, r=bjorn3

Reduce confusion about `make_indirect_byval` by renaming it

As part of doing so, remove the incorrect handling of the wasm target's `make_indirect_byval` (i.e. using it at all).
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Sep 17, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 17, 2024
@workingjubilee
Copy link
Member Author

sigh, forgot to remove the actual wasm revision from the align-byval test, so it just had no directive, so tried to execute on the native...

@workingjubilee
Copy link
Member Author

workingjubilee commented Sep 17, 2024 •
edited
Loading

#130466 is landing a similar test diff for aarch64 so whatever, I'll just wait a bit.

@workingjubilee workingjubilee added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 17, 2024
@workingjubilee workingjubilee force-pushed the these-names-are-indirect branch from 30be609 to 0278430 Compare September 17, 2024 23:21
@bors
Copy link
Contributor

bors commented Sep 18, 2024

☔ The latest upstream changes (presumably #130500) made this pull request unmergeable. Please resolve the merge conflicts.

@workingjubilee
compiler: Use make_indirect for the wasm ABI
0cf89b5 
This is ignored by LLVM, but is still incorrect.
@workingjubilee
compiler: s/make_indirect_byval/pass_by_stack_offset/
a800d1c 
The previous name is just an LLVMism, which conveys almost nothing about
what is actually meant by the function relative to the ABI.

In doing so, remove an already-addressed FIXME.
@workingjubilee
tests: Remove test for wrong wasm codegen
b75711d
@workingjubilee
tests: Move wasm32 to transparent-opaque-ptr.rs test
51718e8
@workingjubilee workingjubilee force-pushed the these-names-are-indirect branch from 0278430 to 51718e8 Compare September 18, 2024 20:15
@workingjubilee workingjubilee removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 18, 2024
@workingjubilee
Copy link
Member Author

effectively identical: https://github.com/davidtwco/rust/blob/9a330742c92c1a87957c0d22fb1f406ff555504c/tests/codegen/repr/transparent-opaque-ptr.rs

@bors r=bjorn3

@bors
Copy link
Contributor

bors commented Sep 18, 2024

📌 Commit 51718e8 has been approved by bjorn3

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 Sep 18, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 18, 2024
@workingjubilee
Rollup merge of rust-lang#130450 - workingjubilee:these-names-are-ind…
3edd952 
…irect, r=bjorn3

Reduce confusion about `make_indirect_byval` by renaming it

As part of doing so, remove the incorrect handling of the wasm target's `make_indirect_byval` (i.e. using it at all).
This was referenced Sep 18, 2024
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2024
@bors
Auto merge of rust-lang#130534 - workingjubilee:rollup-furaug4, r=wor…
a5cf8bb 
…kingjubilee

Rollup of 9 pull requests

Successful merges:

 - rust-lang#97524 (Add `Thread::{into_raw, from_raw}`)
 - rust-lang#127988 (Do not ICE with incorrect empty suggestion)
 - rust-lang#129422 (Gate `repr(Rust)` correctly on non-ADT items)
 - rust-lang#129934 (Win: Open dir for sync access in remove_dir_all)
 - rust-lang#130450 (Reduce confusion about `make_indirect_byval` by renaming it)
 - rust-lang#130476 (Implement ACP 429: add `Lazy{Cell,Lock}::get[_mut]` and `force_mut`)
 - rust-lang#130487 (Update the minimum external LLVM to 18)
 - rust-lang#130513 (Clarify docs for std::fs::File::write)
 - rust-lang#130522 ([Clippy] Swap `manual_retain` to use diagnostic items instead of paths)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b33dd7d into rust-lang:master Sep 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2024
@rust-timer
Unrolled build for rust-lang#130450
282ec54 
Rollup merge of rust-lang#130450 - workingjubilee:these-names-are-indirect, r=bjorn3

Reduce confusion about `make_indirect_byval` by renaming it

As part of doing so, remove the incorrect handling of the wasm target's `make_indirect_byval` (i.e. using it at all).
@workingjubilee workingjubilee deleted the these-names-are-indirect branch September 19, 2024 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bjorn3 bjorn3

Labels
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.83.0
Development

Successfully merging this pull request may close these issues.

WebAssembly calling convention impl incorrectly uses make_indirect_byval()
5 participants
@workingjubilee @bjorn3 @rust-log-analyzer @bors @rustbot