Skip to content

[eddyb's version/leftovers] TypedBuffer / #[spirv(typed_buffer)]. #319

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

Merged
merged 2 commits into from
Jul 4, 2025

Conversation

eddyb
Copy link
Collaborator

@eddyb eddyb commented Jul 4, 2025

This is one of a few branches that (due to my own neglect) got landed in a partial/broken state, this one in:

So what's left here is:

  • some more tests (from @SiebenCorgie, I believe even as a PR on a fork? but that was before I had to remake forks to be based off of this repo etc.)
  • a fix for MaybeUninit<&[T]>, which I kept losing track of as a separate thing until just now!

I believe what happened is #16, in order to get things working, tried passing &mut result_slot to asm!, and that happened to work, and I only figured out why recently (MaybeUninit<&[T]> and &[T] are both scalar pairs, so they each have the same fields, even if they're a different OpTypeStruct SPIR-V types before this PR).

Also, there are some incidental aspects of the tests added here (namely, .array_field[0] that needs to do an effective offset of 0 bytes), which catches #233 (comment).

@eddyb eddyb force-pushed the buffer-interface-block branch from ed8c356 to 8695a3a Compare July 4, 2025 04:24
@eddyb eddyb marked this pull request as draft July 4, 2025 04:25
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.

I have no idea what this change actually fixes. The tests you provided do compile on master without issues. Have you cherry-picked these from edition 2024 branch? Could be that a breaking change was introduced there, that requires these changes.

>,
) {
let mut load_dta = unsafe { my_data.index(global_invocation_id.x as usize) }.some_big_data[0];
load_dta = 32;
Copy link
Member

Choose a reason for hiding this comment

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

Why are we loading data into load_dta: u32 to then immediately replace the value with a const? Could DCE screw us over here?

Also at https://github.com/Rust-GPU/rust-gpu/pull/319/files#diff-734d8d31bf0e2742d96036c34f019f361a6bb9674653012c7e3522c59919f506R16

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question, I wonder if this was meant to be &mut followed by *... = 32;.

Funnily enough, the bug this test catches (post-rust-lang/rust#134117) happens much earlier, so only MIR optimizations could invalidate that part, even in theory (though ideally we'd not rely on incidental stuff like this, and add separate tests).

Anyway, for reading values I tend to just add an output variable to write them to, guaranteeing the read will be kept around, I might make that change if it's not obvious what the original tests meant to do.

@eddyb
Copy link
Collaborator Author

eddyb commented Jul 4, 2025

I have no idea what this change actually fixes. The tests you provided do compile on master without issues.

There's two things going on here (basically the two commits are separate):

abi.rs fix + spirv_std::TypedBuffer deref methods

The version that landed on main (#16) only worked by treating &mut MaybeUninit<&[T]> as if it were &mut &[T], and that only happens to succeed because both of those types happen to be represented as scalar pairs (*T, usize).

That change both deviates from the pattern of result_slot.as_mut_ptr() and is more fragile.

What you were missing when landing that PR (though it's obviously my fault for not getting it landed much sooner) was that the issue you ran into had already been observed, and the fix to abi.rs I had already made on my branch, but you didn't have it in your copy.

Extra TypedBuffer tests

I've only included these tests because I didn't want to throw away @SiebenCorgie's commit, first and foremost.

As an additional bonus, they incidentally happen to trip up #233 (comment) post-rust-lang/rust#134117 (which, yes, only applies to #249, not main).

(also, I tend to prefer landing tests before the changes that break them, if that makes sense)


Have you cherry-picked these from edition 2024 branch?

No, this is the same ancient buffer-interface-block branch, which predates even the TypedBuffer type (hence the weirder name), just rebased on current main, and with the description changed.

From git log buffer-interface-block (plus Name <email> replaced with @github-user):

commit 1188451
Author: @SiebenCorgie
Date: Wed Jul 17 12:54:48 2024 +0200

    add test cases for TypedBuffer

commit 9e640b7
Author: @eddyb
Date: Sat Jan 7 06:26:31 2023 +0200

    [eddyb's version/leftovers] add #[spirv(typed_buffer)] for explicit SpirvType::InterfaceBlocks.

@eddyb eddyb marked this pull request as ready for review July 4, 2025 12:51
@eddyb eddyb enabled auto-merge July 4, 2025 12:52
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.

I'm just gonna approve it as is, if the test in their current weird shape help us debug future code breakage.

@eddyb eddyb added this pull request to the merge queue Jul 4, 2025
@Firestar99 Firestar99 removed this pull request from the merge queue due to a manual request Jul 4, 2025
@eddyb eddyb added this pull request to the merge queue Jul 4, 2025
@Firestar99
Copy link
Member

I wasn't sure if you already disabled merge queue squashing or not

Merged via the queue into Rust-GPU:main with commit 9827e1d Jul 4, 2025
13 checks passed
@eddyb eddyb deleted the buffer-interface-block branch July 4, 2025 14:10
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