Skip to content

Conversation

erikdesjardins
Copy link
Contributor

Currently, this causes a verifier error (https://godbolt.org/z/YYohed4bj), since it uses bitcast, which can't convert between address spaces.

Uncovered due to #105545 (comment)

r? @bjorn3

@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 Dec 11, 2022
Comment on lines 100 to 101
// It doesn't matter precisely how this is codegenned (as a roundtrip through memory or an addrspacecast),
// as long as it doesn't cause a verifier error, e.g. by using `bitcast` (which was the case before).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is codegenned as

%0 = addrspacecast ptr %x to ptr addrspace(1)

I can add a check line instead if you'd prefer

assert_eq!(src.layout.size, dst.layout.size);

// NOTE(eddyb) the `from_immediate` and `to_immediate_scalar`
// conversions allow handling `bool`s the same as `u8`s.
let src = bx.from_immediate(src.immediate());
let src_as_dst = bx.bitcast(src, bx.backend_type(dst.layout));
// LLVM also doesn't like `bitcast`s between pointers in different address spaces.
let src_as_dst = if src_is_ptr {
Copy link
Member

Choose a reason for hiding this comment

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

What if the from type is a pointer and the to type is an integer? Does pointercast work for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't, but that's not possible here due to src_is_ptr == dst_is_ptr above. I can change this line to src_is_ptr && dst_is_ptr if that would be clearer

Copy link
Member

Choose a reason for hiding this comment

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

Right

@bjorn3
Copy link
Member

bjorn3 commented Dec 14, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 14, 2022

📌 Commit 6085d33 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 Dec 14, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2022
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#105399 (Use more LFS functions.)
 - rust-lang#105578 (Fix transmutes between pointers in different address spaces (e.g. fn ptrs on AVR))
 - rust-lang#105598 (explain mem::forget(env_lock) in fork/exec)
 - rust-lang#105624 (Fix unsoundness in bootstrap cache code)
 - rust-lang#105630 (Add a test for rust-lang#92481)
 - rust-lang#105684 (Improve rustdoc markdown variable naming)
 - rust-lang#105697 (Remove fee1-dead from reviewers)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ba71a63 into rust-lang:master Dec 14, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 14, 2022
@erikdesjardins erikdesjardins deleted the addrspacecast branch December 15, 2022 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants