Reduce confusion about make_indirect_byval by renaming it by workingjubilee · Pull Request #130450 · rust-lang/rust (original) (raw)
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.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation16 Commits4 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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?
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
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.
@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.
This comment has been minimized.
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.
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?
...so are we sure the change we want isn't to just skip checking for
byvalinside transparent-struct-ptr.rs?
After some mulling it over I split out that test for wasm.
📌 Commit 30be609 has been approved by bjorn3
It is now in the queue for this repository.
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
bors added a commit to rust-lang-ci/rust that referenced this pull request
…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).
This comment has been minimized.
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
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...
#130466 is landing a similar test diff for aarch64 so whatever, I'll just wait a bit.
This is ignored by LLVM, but is still incorrect.
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.
Status: Blocked on something else such as an RFC or other implementation work.
label
📌 Commit 51718e8 has been approved by bjorn3
It is now in the queue for this repository.
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
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request
…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_byvalby renaming it) - rust-lang#130476 (Implement ACP 429: add
Lazy{Cell,Lock}::get[_mut]andforce_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_retainto use diagnostic items instead of paths)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
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).
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.