Skip to content
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

Add low-level atomic instructions to spirv_std::arch #877

Merged
merged 5 commits into from
Aug 9, 2022

Conversation

BeastLe9enD
Copy link
Contributor

@BeastLe9enD BeastLe9enD commented Jun 1, 2022

Hey, I added some atomic instructions to spirv_std::arch.
There was already a discussion to add them, but it wasn't done at the time because the idea was to implement atomic using core::sync::atomic (see #442).
But I think it's ok to add the intrinsics in advance and then build the API on top of them later 🙂

@BeastLe9enD BeastLe9enD requested a review from eddyb as a code owner June 1, 2022 17:52
#[spirv_std_macros::gpu_only]
#[doc(alias = "OpAtomicFMinEXT")]
#[inline]
pub unsafe fn atomic_f_min_ext<F: Float, const SCOPE: u32, const SEMANTICS: u32>(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just quicky skimming over this on my phone, but I think these would probably be fine with out the _ext on the end of the function name. Everything else looks good though! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I removed the ext suffix 🙂

@BeastLe9enD
Copy link
Contributor Author

I saw that an atomics branch already existed, so I took the documentation from there and added the Number trait as well.
Also, in the long run, it would be better to use the Scope and Semantics enum types, but that would require adt_const_params and generic_const_exprs, which are not stable yet, so I decided to stick with u32 for now

Copy link
Contributor

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

Only noticed one comment nit, but otherwise LGTM.

If @expenses and/or @hrydgard are fine with landing this, feel free to merge or ping me to do so.

crates/spirv-std/src/number.rs Outdated Show resolved Hide resolved
Co-authored-by: Eduard-Mihai Burtescu <edy.burt@gmail.com>
@eddyb
Copy link
Contributor

eddyb commented Aug 9, 2022

@BeastLe9enD I just tried pushing a commit fixing the test, but either the main branch has different rules, or you opted out of the feature, either way, you need to make this change:

diff --git a/tests/ui/arch/debug_printf_type_checking.stderr b/tests/ui/arch/debug_printf_type_checking.stderr
index f719d0aba..d422023e6 100644
--- a/tests/ui/arch/debug_printf_type_checking.stderr
+++ b/tests/ui/arch/debug_printf_type_checking.stderr
@@ -100,9 +100,9 @@ error[E0277]: the trait bound `{float}: Vector<f32, 2_usize>` is not satisfied
               <IVec3 as Vector<i32, 3_usize>>
             and 9 others
 note: required by a bound in `debug_printf_assert_is_vector`
-   --> $SPIRV_STD_SRC/lib.rs:144:8
+   --> $SPIRV_STD_SRC/lib.rs:145:8
     |
-144 |     V: crate::vector::Vector<TY, SIZE>,
+145 |     V: crate::vector::Vector<TY, SIZE>,
     |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `debug_printf_assert_is_vector`

 error[E0308]: mismatched types

(line numbers in spirv-std changed, the test isn't broken, only differs in diagnostic details)

Or cargo compiletest --bless if that's easier for you (i.e. if it won't require a rebuild etc.).


Sorry this fell through the cracks, I don't remember hearing from anyone after posting the above review (#877 (review)) (EDIT: oh, @expenses brought it up, but I missed it in the context of #888).

@eddyb eddyb enabled auto-merge (rebase) August 9, 2022 10:19
auto-merge was automatically disabled August 9, 2022 10:35

Head branch was pushed to by a user without write access

@eddyb eddyb enabled auto-merge (rebase) August 9, 2022 10:58
@eddyb eddyb merged commit ba947ce into EmbarkStudios:main Aug 9, 2022
This was referenced Aug 9, 2022
@oisyn oisyn mentioned this pull request Nov 16, 2022
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