Cast global variables to default address space

[Flakebi]

Pointers for variables all need to be in the same address space for correct compilation. Therefore ensure that even if a global variable is created in a different address space, it is casted to the default address space before its value is used.

This is necessary for the amdgpu target and others where the default address space for global variables is not 0.

For example core does not compile in debug mode when not casting the address space to the default one because it tries to emit the following (simplified) LLVM IR, containing a type mismatch:

@alloc_0 = addrspace(1) constant <{ [6 x i8] }> <{ [6 x i8] c"bit.rs" }>, align 1
@alloc_1 = addrspace(1) constant <{ ptr }> <{ ptr addrspace(1) @alloc_0 }>, align 8
; ^ here a struct containing a `ptr` is needed, but it is created using a `ptr addrspace(1)`

For this to compile, we need to insert a constant addrspacecast before we use a global variable:

@alloc_0 = addrspace(1) constant <{ [6 x i8] }> <{ [6 x i8] c"bit.rs" }>, align 1
@alloc_1 = addrspace(1) constant <{ ptr }> <{ ptr addrspacecast (ptr addrspace(1) @alloc_0 to ptr) }>, align 8

As vtables are global variables as well, they are also created with an addrspacecast. In the SSA backend, after a vtable global is created, metadata is added to it. To add metadata, we need the non-casted global variable. Therefore we strip away an addrspacecast if there is one, to get the underlying global.

Tracking issue: #135024

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @fmease (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[fmease]

r? codegen

[saethlin]

Why does this function call the _impl version? I would think that _impl would be a private helper method or something like that. Unless this means that it's the address of an implementation? The naming here is not doing any favors and there are also no comments on the methods to explain what they are for so I'm really struggling to follow. Can you clarify in the code what the intended difference between static_addr_of and static_addr_of_impl is, and comment why we need the _impl version here?

[Flakebi]

Thanks for taking a look!
I added comments to the functions, let me know if that makes it clearer or if you think something is unclear or missing.

To the point you mentioned here, static_addr_of_impl always returns a llvm::GlobalVariable. But that may not be a ptr (in LLVM IR; this is implicitly an addrspace(0) ptr, 0 is the default and is omitted), but a addrspace(1) ptr.
When we use a global in Rust (or in the ssa codegen), we need it to be just a ptr (so, addrspace(0)). Therefore, static_addr_of must return a ptr and it does that by inserting an addrspacecast on the result of static_addr_of_impl.

So, depending on what we need either function is called. If we need a llvm::GlobalVariable, we need to call static_addr_of_impl, if a ptr is needed, we call static_addr_of.

[saethlin]

That all makes sense, but why in these call sites specifically, is it okay to have addrspace(1) ptr?

[Flakebi]

Here, we want to have the llvm::GlobalValue and edit some of its properties like with llvm::set_value_name below.
We cast it to an (addrspace(0)) ptr with the const_pointercast at the end of this Scalar::Ptr match arm.

[Flakebi]

@nikic, as you reviewed #135025, you may want to take a look at this one as well.

[saethlin]

I think I'm convinced of the implementation here, but this is a rather fiddly change with no tests. Can you add a codegen test that covers the scenarios the implementation is addressing?

[Flakebi]

I think I'm convinced of the implementation here, but this is a rather fiddly change with no tests. Can you add a codegen test that covers the scenarios the implementation is addressing?

Yeah, definitely makes sense. I can do that once the amdgpu target is merged. (Before that, I can't create globals in a different address space.)

[saethlin]

I see. Oof.

@bors r=saethlin

[bors]

📌 Commit b06e840 has been approved by saethlin

It is now in the queue for this repository.