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 countLeadingZeros #2226

Merged
merged 10 commits into from
Jan 31, 2023
Merged

Add countLeadingZeros #2226

merged 10 commits into from
Jan 31, 2023

Conversation

evahop
Copy link
Contributor

@evahop evahop commented Jan 28, 2023

Adds countLeadingZeros from gfx-rs/wgpu#4402

For negative expressions return 0, otherwise return 31 - msb.

This assumes findUMsb, findMSB, and firstbithigh all return an index from the lsb,
but I don't have the means to verify this is actually the case atm.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Behavior looks good!
But the GLSL and HLSL backends write the expr multiple times which won't always work.

The expression needs to either be baked or the function needs to be polyfilled.
I'd go with baking for now since we have the mechanism already implemented for GLSL.

naga/src/back/glsl/mod.rs

Lines 1767 to 1768 in c7d0215

} else if self.need_bake_expressions.contains(&handle) {
Some(format!("{}{}", back::BAKE_PREFIX, handle.index()))

I feel like polyfilling would be a nicer solution but we'd ideally make the transition all at once.
See also #2123 for more details.

@evahop
Copy link
Contributor Author

evahop commented Jan 30, 2023

The expression needs to either be baked or the function needs to be polyfilled. I'd go with baking for now since we have the mechanism already implemented for GLSL.

Okay, I see the issue.
I'm just now noticing FindMSB also has a minimum requirement of 400 while the README.md pegs it at 330+.
Would it be preferable to replace it with something along the lines of es.math.clz32.js? This is also the case for findLSB both of which are currently being used elsewhere.

@evahop
Copy link
Contributor Author

evahop commented Jan 31, 2023

I meant uints in the last commit but I assume it's going to get squashed either way.
Otherwise that should do it, it should address the issue in #2123 (comment),
and I can follow up with a commit if the findMSB/findLSB need replacing.

@evahop
Copy link
Contributor Author

evahop commented Jan 31, 2023

Had some downtime so just went ahead and did it for the scope of this PR and won’t likely have time to address it later.

@teoxoy
Copy link
Member

teoxoy commented Jan 31, 2023

CI is failing due to recently updated clippy lints, we landed a fix in #2229; so we have to rebase this PR.

I'm just now noticing FindMSB also has a minimum requirement of 400 while the README.md pegs it at 330+.
Would it be preferable to replace it with something along the lines of es.math.clz32.js? This is also the case for findLSB both of which are currently being used elsewhere.

Thank you for considering this!
We don't seem to polyfill findMSB/findLSB in the direct translation.
We could introduce a new fn and check if the version supports those functions and if not polyfill; then use it for both, the direct translation and countLeadingZeros.
Similar to how we do it for fma:

if self.options.version.supports_fma_function() {

Had some downtime so just went ahead and did it for the scope of this PR and won’t likely have time to address it later.

Let me know if you can address the comments above. If not, I can rebase it and we can open a new issue for the other change.

Looks great otherwise!

@evahop
Copy link
Contributor Author

evahop commented Jan 31, 2023

I see what you’re pointing out. You can rebase and open another issue. I tried applying the same polyfill to findMSB just now, using the ((clz + 1) % 33) … method on the msl backend, and I’m not sure if its producing the expected behavior, I need to mess with it some more. If I’m not mistaken, seems like clz will always produce a 0 for negative numbers, while findMSB returns an index of the first set 0

src/back/glsl/mod.rs Outdated Show resolved Hide resolved
@teoxoy
Copy link
Member

teoxoy commented Jan 31, 2023

If I’m not mistaken, seems like clz will always produce a 0 for negative numbers, while findMSB returns an index of the first set 0.

This sounds correct, so the msl polyfill is not correct for negative numbers.

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