Move `scalar_to_backend` to ssa #142960

oli-obk · 2025-06-24T12:18:24Z

I think that should make it much more maintainable going forward

rustbot · 2025-06-24T12:18:31Z

Some changes occurred in compiler/rustc_codegen_gcc

Some changes occurred in compiler/rustc_codegen_ssa

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

compiler/rustc_codegen_gcc/src/consts.rs

GuillaumeGomez · 2025-06-24T12:26:03Z

Thanks a lot, it will help making our life easier in the GCC backend!

Just waiting for @antoyo to confirm it's good for him as well then we can r+.

antoyo · 2025-06-24T12:29:53Z

compiler/rustc_codegen_ssa/src/traits/consts.rs

+            Scalar::Int(int) => {
+                let data = int.to_bits(layout.size(self));
+                let llval = self.const_uint_big(self.type_ix(bitsize), data);
+                if matches!(layout.primitive(), Primitive::Pointer(_)) {
+                    self.const_int_to_ptr(llval, llty)
+                } else {
+                    self.const_bitcast(llval, llty)
+                }
+            }


This is very likely to break cg_gcc since the bitcast doesn't work in const context (see comment in the original implementation).
I can check if I could make it work in const context in libgccjit if needed.

Nice refactor, btw!

can we cheat and do the bitsize > 1 && ty.is_integral() && bytesize as u32 == ty.get_size() check within the cg_gcc const_bitcast impl?

I'll have to thing about whether this could break other things: if that ever gets called with a value of a struct type, for instance, I assume this would break.
Would you have access to bytesize?

we can very readily provide it, we're in control of this method and llvm will just ignore it

Let me try locally since I'm concerned that this will break other things.

we can very readily provide it, we're in control of this method and llvm will just ignore it

So, your plan would be to change all other calls and possibly the API of the functions calling const_bitcast to add the missing parameters if needed?
Or will you change the other calls so that they call another function than const_bitcast?

no rush, we can let this PR sit for a while.

So, your plan would be to change all other calls and possibly the API of the functions calling const_bitcast to add the missing parameters if needed?
Or will you change the other calls so that they call another function than const_bitcast?

ah yea we should probably just add a new function for this specific case and have llvm forward to const_bitcast and gcc do the checks and fallback logic

Ok, good.
We're in the process of adding more tests for cg_gcc in the Rust CI (will try to open an MCP for this soon), so this should make this kind of refactor much easier.

do you mean by this comment you'd prefer to block this PR on that landing?

bjorn3 · 2025-06-24T13:19:41Z

compiler/rustc_codegen_ssa/src/traits/consts.rs

+    /// Create a global constant.
+    ///
+    /// The returned global variable is a pointer in the default address space for globals.
+    fn static_addr_of_impl(&self, cv: Self::Value, align: Align, kind: Option<&str>)


Maybe call this static_addr_of_const?

Also it looks like the kind argument is always None, so you can remove it.

unfortunately vtables need this at present to achieve

rust/tests/codegen/debug-vtable.rs

Line 16 in a5fa12b

// Make sure that vtables don't have the unnamed_addr attribute when debuginfo is enabled.

and since vtables are const, and that logic calls the mut logic before marking the alloc as const, we need to keep it around everywhere. There's likely things to improve here, but that should probably be its own PR

ah fun, we only do that in some vtable paths, but not in the one in scalar_to_backend

bjorn3 · 2025-06-24T13:20:45Z

compiler/rustc_codegen_ssa/src/traits/consts.rs

+    -> Self::Value;
+
+    /// Same as `static_addr_of_impl`, but does not mark the static as immutable
+    fn static_addr_of_mut(&self, cv: Self::Value, align: Align, kind: Option<&str>) -> Self::Value;


Same here. kind is always None.

compiler/rustc_codegen_ssa/src/traits/consts.rs

bors · 2025-06-29T08:23:45Z

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

bors · 2025-07-01T03:23:58Z

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

bors · 2025-07-07T21:25:41Z

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

bors · 2025-07-10T11:07:42Z

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

eddyb

Mostly just happy to see the separation between static_addr_of and const_data_from_alloc disappear (after noticing myself that pattern and confirming that all calls to either contains the other), the comments are small things I noticed in passing (of varying subjectivity).

(Oh, and since I can't leave a comment on a line outside the diff: const_struct is unused in rustc_codegen_ssa, and AFAICT it's also only ever used to build the initializers for globals, not any SSA values)

eddyb · 2025-07-18T09:03:57Z

compiler/rustc_codegen_ssa/src/traits/consts.rs

+    /// Create a global constant.
+    ///
+    /// The returned global variable is a pointer in the default address space for globals.
+    fn static_addr_of_const(&self, alloc: ConstAllocation<'_>, kind: Option<&str>) -> Self::Value;

-    fn scalar_to_backend(&self, cv: Scalar, layout: abi::Scalar, llty: Self::Type) -> Self::Value;
+    /// Same as `static_addr_of_const`, but does not mark the static as immutable
+    fn static_addr_of_mut(&self, alloc: ConstAllocation<'_>, kind: Option<&str>) -> Self::Value;


~~Why would these be different from static_addr_of? This feels like an accidental historical divergence within rustc_codegen_llvm, from what I could tell.~~

EDIT: looks like there used to be static_addr_of and static_addr_of_mut before e8da3c6

I think a single static_addr_of method (not here but in StaticCodegenMethods, where it is right now) that either takes Mutability, or even better inspects alloc.inner().mutability itself, would be an improvement over having 3 slightly different static_addr_of* methods.

eddyb · 2025-07-18T09:10:27Z

compiler/rustc_codegen_ssa/src/traits/consts.rs

+        let bitsize = if layout.is_bool() { 1 } else { layout.size(self).bits() };
+        match cv {
+            Scalar::Int(int) => {
+                let data = int.to_bits(layout.size(self));
+                let val = self.const_uint_big(self.type_ix(bitsize), data);


I feel like spelling cx.type_bool() using cx.type_ix(1) is a LLVM-ism, and generally the only integer types a codegen backend is required to support, are the ones matching abi::Integer.

But I don't have a lot of good suggestions, other than e.g. spelling type_ix as type_int (and making it take Size, at least, so it's harder to misuse), which wouldn't be trivial because type_int is already taken (for c_int AFAICT?).

eddyb · 2025-07-18T09:11:13Z

compiler/rustc_codegen_ssa/src/traits/consts.rs

+    /// Same as `static_addr_of_const`, but does not mark the static as immutable
+    fn static_addr_of_mut(&self, alloc: ConstAllocation<'_>, kind: Option<&str>) -> Self::Value;
+
+    fn scalar_to_backend(&self, cv: Scalar, layout: abi::Scalar, ty: Self::Type) -> Self::Value {


Been looking at this method a lot and the name keeps feeling out of place, what do you think about naming it const_scalar?

eddyb · 2025-07-18T09:25:35Z

compiler/rustc_codegen_ssa/src/traits/statics.rs

-    fn static_addr_of(&self, cv: Self::Value, align: Align, kind: Option<&str>) -> Self::Value;
+    fn static_addr_of(&self, alloc: ConstAllocation<'_>, kind: Option<&str>) -> Self::Value;
+    fn get_value_name(&self, val: Self::Value) -> &[u8];
+    fn set_value_name(&self, val: Self::Value, name: &[u8]);


Why are these here? The non-constant one is treated as a debuginfo helper:

rust/compiler/rustc_codegen_ssa/src/traits/debuginfo.rs

Line 85 in 6c0a912

fn set_var_name(&mut self, value: Self::Value, name: &str);

eddyb · 2025-07-18T09:25:37Z

compiler/rustc_codegen_ssa/src/traits/statics.rs

    fn codegen_static(&mut self, def_id: DefId);
+    fn get_static(&self, def_id: DefId) -> Self::Value;


Can remove the other copy of this method from StaticBuilderMethods (and that entire trait, since it would be empty).

I think I tried and there were some issues, but I'll try again

eddyb · 2025-07-18T09:28:59Z

compiler/rustc_codegen_ssa/src/traits/consts.rs

+    fn const_bitcast(&self, val: Self::Value, ty: Self::Type) -> Self::Value;
+    fn const_pointercast(&self, val: Self::Value, ty: Self::Type) -> Self::Value;
+    fn const_int_to_ptr(&self, val: Self::Value, ty: Self::Type) -> Self::Value;
+    fn const_ptr_to_int(&self, val: Self::Value, ty: Self::Type) -> Self::Value;


Is there any consensus on whether these should be separate operations, or could all be subsumed by const_bitcast? (I'm wondering along similar lines to ptr as usize vs ptr->int transmute)

if they are all subsumed then some backends need to check their type, even if the static logic knows what it actually wants

eddyb · 2025-07-18T09:37:32Z

compiler/rustc_codegen_ssa/src/traits/consts.rs

+                let base_addr_space = global_alloc.address_space(self);
+
+                // Cast to the required address space if necessary
+                let val = self.const_pointercast(base_addr, self.type_ptr_ext(base_addr_space));


Wouldn't base_addr_space already be the AS of base_addr?

I guess not, e.g. if the pointer starts off in some "AS of globals" and then needs to be cast to the default AS, which is what base_addr_space would refer to.

Wish I had any suggestions for what to name this to better express that it's an "intermediate" between base_addr (binning its AS into one of the two that Rust itself supports, at most - i.e. data-vs-code) and layout/ty (which can, in theory, be using a different AS, or not even be a pointer at all, etc.).

eddyb · 2025-07-18T09:39:44Z

compiler/rustc_codegen_ssa/src/mir/operand.rs

@@ -238,8 +238,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
                // Neither a scalar nor scalar pair. Load from a place
                // FIXME: should we cache `const_data_from_alloc` to avoid repeating this for the
                // same `ConstAllocation`?
-                let init = bx.const_data_from_alloc(alloc);
-                let base_addr = bx.static_addr_of(init, alloc_align, None);
+                let base_addr = bx.static_addr_of(alloc, None);


This is in from_const_alloc, which could be adjusted to call scalar_to_backend with a Scalar::Ptr.

(At the very least, the comment mentioning const_data_from_alloc should be updated)

eddyb · 2025-07-18T09:42:56Z

compiler/rustc_codegen_ssa/src/meth.rs

    let vtable_allocation = tcx.global_alloc(vtable_alloc_id).unwrap_memory();
-    let vtable_const = cx.const_data_from_alloc(vtable_allocation);
-    let align = cx.data_layout().pointer_align().abi;
-    let vtable = cx.static_addr_of(vtable_const, align, Some("vtable"));
+    let vtable = cx.static_addr_of(vtable_allocation, Some("vtable"));


Looks like a historical accident that this is separate from scalar_to_backend's own ability to handle vtables (and only one of them is cached?) - AFAICT resulting in different codegen for:

let x = (); &x as &dyn Trait

const { &() as &dyn Trait }

GuillaumeGomez · 2025-07-18T15:12:23Z

compiler/rustc_codegen_gcc/src/consts.rs

+        // TODO(antoyo)
+        &[]


Finally added the missing feature in rust-lang/gcc#77 and rust-lang/gccjit.rs#54. So can be done in this PR or I can send a follow-up once merged to add:

Suggested change

// TODO(antoyo)

&[]

val.get_name().unwrap().as_slice()

That would require updating the GCC submodule, so perhaps it is better in a follow-up PR.

GuillaumeGomez · 2025-07-18T15:12:36Z

compiler/rustc_codegen_gcc/src/consts.rs

+        &[]
+    }
+    fn set_value_name(&self, _val: Self::Value, _name: &[u8]) {
+        // TODO(antoyo)


Now I need to do this one. ^^'

rustbot assigned GuillaumeGomez Jun 24, 2025

GuillaumeGomez reviewed Jun 24, 2025

View reviewed changes

compiler/rustc_codegen_gcc/src/consts.rs Show resolved Hide resolved

GuillaumeGomez approved these changes Jun 24, 2025

View reviewed changes

antoyo reviewed Jun 24, 2025

View reviewed changes

bjorn3 reviewed Jun 24, 2025

View reviewed changes

compiler/rustc_codegen_ssa/src/traits/consts.rs Outdated Show resolved Hide resolved

oli-obk force-pushed the scalar-to-backend-dedup branch from 35ccb26 to 84c8bde Compare June 30, 2025 09:07

oli-obk force-pushed the scalar-to-backend-dedup branch from 84c8bde to c285206 Compare July 2, 2025 13:51

This comment has been minimized.

Sign in to view

oli-obk force-pushed the scalar-to-backend-dedup branch from c285206 to ed01d01 Compare July 2, 2025 14:21

oli-obk force-pushed the scalar-to-backend-dedup branch from ed01d01 to 74f9659 Compare July 8, 2025 07:37

oli-obk mentioned this pull request Jul 9, 2025

Support using const pointers in asm const operand #138618

Open

oli-obk added 3 commits July 10, 2025 14:32

Move scalar_to_backend to ssa

5567b2a

Rename static_addr_of_impl

5855748

Remove const_data_from_alloc to make cg_ssa more amenable for cg_clif

e02729a

oli-obk force-pushed the scalar-to-backend-dedup branch from 74f9659 to e02729a Compare July 10, 2025 14:37

eddyb reviewed Jul 18, 2025

View reviewed changes

GuillaumeGomez reviewed Jul 18, 2025

View reviewed changes

		fn codegen_static(&mut self, def_id: DefId);
		fn get_static(&self, def_id: DefId) -> Self::Value;

Move scalar_to_backend to ssa #142960

Are you sure you want to change the base?

Move scalar_to_backend to ssa #142960

Conversation

oli-obk commented Jun 24, 2025

Uh oh!

rustbot commented Jun 24, 2025

Uh oh!

Uh oh!

GuillaumeGomez commented Jun 24, 2025

Uh oh!

antoyo Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoyo Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bors commented Jun 29, 2025

Uh oh!

bors commented Jul 1, 2025

Uh oh!

This comment has been minimized.

bors commented Jul 7, 2025

Uh oh!

bors commented Jul 10, 2025

Uh oh!

eddyb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Move `scalar_to_backend` to ssa #142960

Move `scalar_to_backend` to ssa #142960

antoyo Jun 24, 2025 •

edited

Loading

antoyo Jun 24, 2025 •

edited

Loading