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 FindLsb / FindMsb #1473

Merged
merged 7 commits into from
Dec 20, 2021
Merged

Add FindLsb / FindMsb #1473

merged 7 commits into from
Dec 20, 2021

Conversation

fintelia
Copy link
Contributor

Currently untested. I welcome feedback on whether this approach makes sense and how to go about adding tests.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you!
The way to test the backends here would be to add these function calls to tests/in/bits.wgsl.

src/back/msl/writer.rs Outdated Show resolved Hide resolved
@fintelia
Copy link
Contributor Author

fintelia commented Nov 6, 2021

Ok, it seems that passing validation requires that all GLSL inputs can be written as valid WGSL. Unfortunately, WGSL doesn't yet have ctz / clz functions so I'm not sure the right way to achieve that without simply not testing the new functionality

src/back/wgsl/writer.rs Outdated Show resolved Hide resolved
@kvark
Copy link
Member

kvark commented Nov 9, 2021

I agree about the naming, it would be nice to settle this in the group.

@fintelia
Copy link
Contributor Author

Ok, at this point it seems that the main blocker is that WGSL's builtins for this functionality haven't been specified yet. What would be the right way to push that forward so this PR can be merged?

@kvark
Copy link
Member

kvark commented Nov 25, 2021

If you want to use it now, we can land this with the expectation that we'll rename these methods once the group settles on them.

@fintelia fintelia force-pushed the find-lsb-msb branch 4 times, most recently from 39506a3 to 1550ba5 Compare November 30, 2021 03:11
@kvark
Copy link
Member

kvark commented Dec 16, 2021

@fintelia there seem to be a small problem left on Metal?

@kvark kvark mentioned this pull request Dec 17, 2021
@fintelia
Copy link
Contributor Author

Ok, I think I managed to fix it. The issue was that there needed to be an unsigned integer -> signed integer cast, but to do that the vector width of the input needed to be known. I tried a bunch of ways to cause the type coercion to produce that without the generated code differing between them, but none ended up working. The current version checks the input width so it can insert a cast using one of int/int2/int3/int4.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you!

src/back/msl/writer.rs Outdated Show resolved Hide resolved
src/back/msl/writer.rs Outdated Show resolved Hide resolved
src/proc/typifier.rs Outdated Show resolved Hide resolved
tests/in/bits.wgsl Outdated Show resolved Hide resolved
@kvark kvark merged commit f9b3485 into gfx-rs:master Dec 20, 2021
@kvark
Copy link
Member

kvark commented Jan 11, 2022

@fintelia would you be interested to update the naming according to gpuweb/gpuweb#2467 ?

@fintelia
Copy link
Contributor Author

I probably wouldn't be able to get to it for a while. If anyone else wants to make the update they should go ahead!

With the functions now fully specified in WGSL, someone should probably also make sure there's conformance tests for the various edge cases. I'm not fully confident that I understood the specs fully for each of the backends; testing on lots of platforms is the only way to be sure.

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.

2 participants