Skip to content
This repository was archived by the owner on Oct 31, 2025. It is now read-only.

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.

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants