Skip to content

rustup: update to nightly-2025-05-09 (~1.88) and Rust 2024 edition. #249

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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

eddyb
Copy link
Collaborator

@eddyb eddyb commented May 12, 2025

This PR supersedes (and includes many of the changes from):


This brings us 4 whole Rust releases ahead, to a 1.88-equivalent nightly.
(Rust 1.88.0 stable will be released on June 26th, 2025)

The many commits exist in part because of nightly bisection (to find nightly-2024-12-16, containing the GEP array change), but also because there have been several changes of note:

  • target specs no longer spuriously claim an OS of unknown
    • (the sole unknown component in e.g. spirv-unknown-vulkan1.1 is the vendor, whereas the OS is none and omitted in the "triple")
  • maybe_inbounds_gep had to be made more flexible, due to:
    Modifies the index instruction from gep [0 x %Type] to gep %Type rust-lang/rust#134117
    • (supporting that pattern was achieved by attempting pointercast first, which can itself produce a "first element of array" GEP that can then be merged with an "intra-array indexing GEP")
  • f16/f128 required some stubbing out (i.e. intrinsics)
    • it may be possible to support f16, but I've not attempted that in this PR
  • had to re-enable niches for bool (i.e. making Option<bool> byte-sized), due a transmute (from ControlFlow<bool> to u8) added to core::cmp in:
    Use the chaining methods on PartialOrd for slices too rust-lang/rust#138881
    • not sure Option<bool> ever worked in Rust-GPU (the comments I found are confusing, I thought it had users and/or tests, but maybe not)
  • (the remaining ones have to do w/ rustc_codegen_ssa patching)
  • rustc_codegen_spirv had to be switched to the Rust 2024 edition (as rustc_codegen_ssa had done the same upstream)
  • old-style #[repr(simd)] (using individual struct fields) has had even more support ripped out upstream
    • hooking/patching that support back in was possible, but it's clearly not an intended feature and we should try to move off of it sooner than later
  • some dependencies of rustc_codegen_ssa::back (i.e. object) required more patching to remove their use sites
    • only needed because nightlies have had two prebuilt versions of object, breaking extern crate object; (as it may load the wrong version)
    • this was also the original blocker for rustup to nightly-2025-04-28 #246
      (before all the other issues were discovered through testing)
    • @LegNeato has been separately working on removing that duplication upstream

While not necessary, this (just like #246) includes a full migration of the entire workspace to Rust 2024 (leading to lots of unsafe {...} in unsafe fns in spirv-std, mainly) - we can split that out if it seems better, but I at least tried to keep it in its own commit.

@eddyb eddyb enabled auto-merge May 12, 2025 18:40
@eddyb eddyb force-pushed the rustup-1.88 branch 2 times, most recently from 88fd99e to 554d850 Compare May 12, 2025 19:24
@Firestar99
Copy link
Member

Thanks for getting us up to the newest versions! 🎉
I'm currently away from home, so I'll likely won't be able to review until next week.

Copy link
Collaborator

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

Some minor comments. I liked how many of your changes are better than mine ha. Only rejecting so it doesn't merge and others can review.

@@ -276,6 +317,12 @@ mod maybe_pqp_cg_ssa;

println!("cargo::rustc-check-cfg=cfg(rustc_codegen_spirv_disable_pqp_cg_ssa)");

// HACK(eddyb) `if cfg!(llvm_enzyme)` added upstream for autodiff support.
println!("cargo::rustc-check-cfg=cfg(llvm_enzyme)");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh cool, I didn't know you could put those here.

// --- Attempt GEP Merging Path ---

// Check if the base pointer `ptr` itself was the result of a previous
// AccessChain instruction. Merging is only attempted if the input type `ty`
// matches the pointer's actual underlying pointee type `original_pointee_ty`.
// If they differ, merging could be invalid.
// HACK(eddyb) always attempted now, because we `pointercast` first, which:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just update the comment rather than having a new comment modify...we have git 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be clear, when I do this it's because I'm already several steps deep and accurately rewording comments might not be the easiest/quickest thing at that moment (but I agree I could/should fix it before landing the PR).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or just delete the comment!

@@ -908,12 +910,21 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// 2. The *last* index of the original AccessChain is a constant.
// 3. The *first* index (`ptr_base_index`) of the *current* GEP is a constant.
// Merging usually involves adding these two constant indices.
//
// FIXME(eddyb) the above comment seems inaccurate, there is no reason
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, just fix or delete the comment.

@eddyb eddyb disabled auto-merge May 14, 2025 07:54
@Firestar99
Copy link
Member

Did a quick test of this branch and get a ton of:

error: OpPtrNotEqual without OpCapability VariablePointers
     --> /home/firestar99/.rustup/toolchains/nightly-2025-05-09-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/mem/maybe_uninit.rs:619:38
      |
  619 |             ManuallyDrop::into_inner(self.value)
      |                                      ^^^^^^^^^^
      |
  note: used from within `<core::mem::maybe_uninit::MaybeUninit<&[u32]>>::assume_init`
     --> /home/firestar99/.rustup/toolchains/nightly-2025-05-09-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/mem/maybe_uninit.rs:619:38
      |
  619 |             ManuallyDrop::into_inner(self.value)
      |                                      ^^^^^^^^^^
  note: called by `<spirv_std::typed_buffer::TypedBuffer<[u32]> as core::ops::deref::Deref>::deref`
     --> /home/firestar99/.cargo/git/checkouts/rust-gpu-d06d15e2ba0f0ae2/ea25c7e/crates/spirv-std/src/typed_buffer.rs:79:13
      |
  79  |             result_slot.assume_init()
      |             ^^^^^^^^^^^^^^^^^^^^^^^^^
  note: called by `<&rust_gpu_bindless_shaders::descriptor::descriptors::Descriptors as rust_gpu_bindless_shaders::descriptor::descriptors::DescriptorAccess<rust_gpu_bindless_shaders::descriptor::buffer::Buffer<[rust_gpu_bindless_egui_shaders::vertex::Vertex]>>>::access::<rust_gpu_bindless_shaders::descriptor::transient::Transient>`
     --> /home/firestar99/workspace/space-rust/rust-gpu-bindless/crates/shaders/src/descriptor/descriptors.rs:55:36
      |
  55  |         unsafe { BufferSlice::from_slice(self.buffers.index(desc.id().index().to_usize()), self.meta) }
      |                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[...]
  error: cannot cast between pointer types
         from `*void`
           to `*[u32]`
     --> /home/firestar99/.rustup/toolchains/nightly-2025-05-09-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/mem/maybe_uninit.rs:619:38
[...] // same backtrace

Also when you first update to this PR, cargo errors telling you that your rustc version is too old for edition 2024, cause it's still locked to the master nightly. I usually figure out what the new nightly is by running it, making updating a bit weird, as I first need to switch to eg. stable just to get the correct version displayed. Just something to watch out for, as other people updating may experience this over the next few months. (Except cargo gpu users, for them it should be flawless)

@eddyb
Copy link
Collaborator Author

eddyb commented May 16, 2025

cargo errors telling you that your rustc version is too old for edition 2024

Oh, that's cursed, I wish we could completely avoid edition = "2024" for any crates (and do that migration separately), but:

  • how do we deal with the (eventual) release, same problem there, right?
    • (even if we do a separate release that just bumps the edition, some users might update from 0.9 to a Rust-GPU that needs Rust 2024, and get that issue anyway)
  • it would require more rustc_codegen_ssa patching to account for the edition differences (as upstream rustc fully switched to Rust 2024 at some point)
    • maybe not hard, but could get finnicky and weird (IIRC pattern binding ergonomics rules got tightened so some &/ref patterns needed rewrites)

As for the pointer errors: if that's some straight-forward TypedBuffer usage, it really shouldn't be able to break like that, without also breaking tests, was it landed without tests, or are they too simple perhaps?

(If you have the patience, you could probably use some of the commits to bisect at least among the nightlies I had passing all tests, to narrow down the breakage - or I could try building your project myself, if that's simpler overall etc.)

@LegNeato
Copy link
Collaborator

I don't think we have any users on 0.9 so I wouldn't worry about it. We can just document what is going on, and cargo-gpu will help eventually.

@Firestar99
Copy link
Member

Firestar99 commented May 17, 2025

I don't think we have any users on 0.9...

Oh you wouldn't believe the amount of people that prefer to use a 2 year old outdated version over the master of a repo 😅
Luckily, cargo gpu is completely unaffected by any of it! 🎉
(already tested this version, just requires bumping the spirv_builder dependency to pick up the new target jsons)
see later comment on cargo gpu

@Firestar99
Copy link
Member

Firestar99 commented May 17, 2025

I can have a look at bisecting the error or making a minimal repo, though currently I'm slightly sneezy. But if you already want to take a look, changing the rev in the root workspace and cargo c should do it

@Firestar99
Copy link
Member

bisected it for you:

@Firestar99
Copy link
Member

cargo gpu: This is the first PR to update the target spec jsons, which causes cargo gpu to run into some trouble. The codegen backend verifies that these jsons match exactly what it expects, and there's no flag to skip that check, and adding one now wouldn't help since we want to keep compatible with at least 0.9.0. Cargo gpu was never designed to have multiple target-spec versions, it always just used the latest. Now we need to manage two different target spec json versions if we want to be compatible with before and after this change. We also can't easily extract the json from the rustc_codegen_spirv crate, as it's only contained in the compatible spirv-builder crate. So we end up in this weird state where we are likely forced to have per-codegen backend target specs (which is fine) and manage multiple target-spec versions internally and hopefully select the correct one depending on rustc version (not so nice).
cc @schell @tombh

@Firestar99
Copy link
Member

cargo gpu solution: Could we move the target jsons from spirv_builder to rustc_codegen_spirv-types? That would allow cargo-gpu to resolve the required target jsons from rustc_codegen_spirv depending on rustc_codegen_spirv-types (and spirv_builder will retain access to it). If we can't find any target jsons in rustc_codegen_spirv-types, we can just fall back on the current target jsons, which we'd have to ship with cargo-gpu for the forseeable future. But I think that's a fine compromise, at least we wouldn't need to ship all future target jsons as well.

That would require merging this move of target jsons before this PR is merged, to not have a hole of unsupported commits on master.

I'll have a look at implementing this tomorrow.

Copy link
Member

@Firestar99 Firestar99 left a comment

Choose a reason for hiding this comment

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

changing target jsons breaks cargo gpu, see #249 (comment)
(just to prevent this from merging)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants