add support for leading_zeros, trailing_zeros and fix count_ones #213
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Sweet! I actually looked at this too, just didn't put my WIP up. Differences between what I did:
I pushed a branch. I think the code is largely correct, but it is only lightly tested: |
This runs wgsl shaders and rust shaders and compares the output. If the output differs, the test fails. Differential testing is better than snapshot testing or golden file testing as there are no reference files to get outdated. Note that we are only using wgpu for now, but I want to also vary the host-side crate / use `ash`. At least we are using `wgpu`'s vulkan support on linux, so we are not just testing naga's translation.
Without this it waits on the package lock anyway
Looks like this option isn't there anymore?
Firestar99
force-pushed
the
leading_trailing_zeros
branch
from
702cb97 to
c53e03c
Compare
Firestar99
changed the title
|
This is ready for review, but completely untested beyond compile tests. Would love some integration tests or fuzzing right now, as it's quite easy to screw up bitcasts or misread documentation. But I guess some careful reviewing must do. |
|
You could rebase on #216 ? |
Firestar99
force-pushed
the
leading_trailing_zeros
branch
from
10f0283 to
c0c5879
Compare
|
Sure, there you go a rebase, git could just do it automatically. I assume CI will now do run some difftests? |
|
Yep! Now you can add a runtime diff test for this if you want. But I'll also be revamping a bit and squashing that PR, so I don't want to make your git stuff more annoying. Up to you if it is worth it or want to land as-is! |
| } | ||
| .with_type(ty) | ||
| } | ||
| _ => self.fatal("count_ones on a non-integer type"), |
There was a problem hiding this comment.
Nit: wrong function name in comment
| _ => { | ||
| let undef = self.undef(ty).def(self); | ||
| self.zombie(undef, &format!( | ||
| "counting leading / trailing zeros on unsupported {ty:?} bit integer type" |
There was a problem hiding this comment.
Nit: wrong function name in comment
| let lower = self.emit().s_convert(u32, None, arg).unwrap(); | ||
| let higher = self | ||
| .emit() | ||
| .shift_left_logical(ty, None, arg, u32_32) |
There was a problem hiding this comment.
I think this should be shift_right_logical ?
In main
leading_zerosandtrailing_zerosrequire some obscure Intel Extension. This PR changes them to use theGLSL.std.450functionsFindILsbandFindUMsb, which do the exact same thing. Though they slightly mismatch when passing in 0, rust expects the bit count of the type but they return!0/-1. And they require to be used exclusively with 32bit integers, so some careful type and bitcasting is required to emulate it for all other types, like u8, u16 and u64.Also fixes
count_onesandbit_reverse, as VUID-StandaloneSpirv-Base-04781 also requires their args to always be 32bit integers as well but we were ignoring it.While removing the Intel extension, some code managing enabled extensions was triggering an unused warning, so I removed it in 1fb0301 which we may revert if needed.
Closes #210 #215