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

wgsl: Change *Unbounded[Interval|Vector|...] to *Infinite[Interval|Vector|...] #3208

Closed
wants to merge 2 commits into from

Conversation

zoddicus
Copy link
Contributor

@zoddicus zoddicus commented Dec 5, 2023

The current 'unbounded' constants are for [-∞, ∞] intervals. Changing
the name to avoid confusion once the 'bounded' state bit is added to
the interval class.

Issue #3201


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

@zoddicus zoddicus added the wgsl label Dec 5, 2023
@zoddicus zoddicus self-assigned this Dec 5, 2023
@zoddicus
Copy link
Contributor Author

zoddicus commented Dec 5, 2023

This is dependent on the bounds -> endpoints change, so new code is in the second diff, bd95de9

Copy link
Contributor

@jiangzhaoming jiangzhaoming left a comment

Choose a reason for hiding this comment

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

The code change generally looks good to me, thanks!

But I am not sure about the usage of name infinite. If I get it right, we use "unbounded interval" to indicate that we accept any result of the given FP type, or as the WGSL spec said "indeterminate value". But technically yes, it is a interval of [-inf, inf].

I hope others can have comments on this.

src/webgpu/util/floating_point.ts Outdated Show resolved Hide resolved
@jiangzhaoming
Copy link
Contributor

The code change generally looks good to me, thanks!

But I am not sure about the usage of name infinite. If I get it right, we use "unbounded interval" to indicate that we accept any result of the given FP type, or as the WGSL spec said "indeterminate value". But technically yes, it is a interval of [-inf, inf].

I hope others can have comments on this.

@ben-clayton @dneto0

@zoddicus
Copy link
Contributor Author

zoddicus commented Dec 6, 2023

For context, in PRs that depend on this change I am currently working on a rework of how OOB behaviour is tracked in the framework.

Specifically instead of depending on checking the intervals bounds at the end of the calculation, I am adding a bit to the state of the interval that indicates whether or not it was created as the result of a calculation that went OOB, and this bit propegates, so if an earlier calculation goes OOB, but a later one pulls it back in bounds, there will still be the 'correct' bounds calculated, but the fact the result could be indeterminate will be preserved.

In my prototype these usage of the Unbounded/Infinite interval constant drops off noticeabley, except for specific circumstantes like division by zero. The term 'unboundedInterval' becomes confusing because one has to distinguish between a regular FPInterval with the OOB bit set, which is unbounded for the purposes of checking expectations, and this constant interval with infinite endpoints.

(I have split up my work changes so that PRs like this can be reviewed seperately/earlier, since I am not sure if I will have the entire change set ready before the winter break)

@ben-clayton
Copy link
Contributor

I don't have strong opinions about the terminology here, but would like to better understand the new bit being added to the interval. @zoddicus - do you have anything (bug for example) that explains why we need to add this bit? Ideally, something that explains what isn't working right now, and how the bit fixes it?

@zoddicus
Copy link
Contributor Author

zoddicus commented Dec 6, 2023

This is the issue that I have filed, #3139

In brief, for mix specifically, I have written a test case that goes OOB in one of the intermediate steps, but comes back in bounds by the end. This leads to a situation where (because of the intermediate OOB) the CTS should accept any input, but because we have an in bounds end interval, it is checking against that.

As implemented the framework is supposed to have converted that OOB value to the infinite interval at the intermediate step (which I probably could find the bug and fix) but that would eliminate the endpoint values. For validation there is distinction between going OOB near the edge of the range versus being way out there, so retaining what the actual calculated endpoints are is going to be needed for the validation testing.

@zoddicus
Copy link
Contributor Author

zoddicus commented Dec 6, 2023

The bit should resolve this issue by allowing .contains to be rewritten so that if bit == true return true at the beginning.

@ben-clayton
Copy link
Contributor

In brief, for mix specifically, I have written a test case that goes OOB in one of the intermediate steps, but comes back in bounds by the end. This leads to a situation where (because of the intermediate OOB) the CTS should accept any input, but because we have an in bounds end interval, it is checking against that.

This is roughly what I understood, but I'm vague about:
(a) How it goes OOB and then back into bounds.
(b) Why the modeling of this out-then-in-bounds doesn't accurately replicate the desired outcome.

Would it be possible to describe this in steps, with a concrete example, highlighting where the mismatch occurs?

@zoddicus
Copy link
Contributor Author

zoddicus commented Dec 6, 2023

Took me a moment to come up with a solid example, because my new workstation apparently does the math in a higher precision, so wasn't causing the CTS to trip anymore 🙃

For mix one of the allowed formulas is e1 * (1 - e3) + e2 * e3
If we use these inputs

e1 = 1                                                                                                                                           
e2 = 2                                                                                                                                           
e3 = (max/2) + 1

We have the following operations:
1)

e2 * e3 = 2 * (max/2) + 2                                                                                                                        
        = max + 2
1 - e3 = 1 - max/2 + 1                                                                                                                           
       = -max / 2 
e1 * (1 - e3) = 1 * ( -max/2)                                                                                                                    
              = -max/2
e1 * (1 - e3) + e2 * e3 = -max/2 + (max + 2)                                                                                                     
                        = max/2 + 2

So the final result 4) is in bounds, but 1) went OOB, and if we are operating in f16, number in TS can accurately represent all of this.

The framework for f16 currently returns [32752.0, 32768.0], which is in bounds. The current implementation isn't behaving as intended, because at 1) this should have be splatted to [-∞, ∞], which should have propegated through. This in theory could be fixed, but will need to be revisited for validation testing.

Specifically https://www.w3.org/TR/WGSL/#floating-point-overflow states that if a value is OOB, but near the edge of the range, than it is fair for a platform to round into range, so doesn't nescarily cause a validation error. So how far things go OOB needs to be tracked, so this unbounded bit will probably need to be expanded to be a tri-state, always in bounds, always alteast near OOB, or went far OOB.

…ctor|...]

The current 'unbounded' constants are for [-∞, ∞] intervals. Changing
the name to avoid confusion once the 'bounded' state bit is added to
the interval class.

Issue gpuweb#3201
@zoddicus zoddicus force-pushed the renameUnboundedInterval branch from bd95de9 to 8e36b80 Compare December 6, 2023 20:01
@zoddicus zoddicus force-pushed the renameUnboundedInterval branch from 26d1ec2 to 8ca165a Compare December 6, 2023 20:08
@ben-clayton
Copy link
Contributor

Thank you for the example!

The framework for f16 currently returns [32752.0, 32768.0], which is in bounds. The current implementation isn't behaving as intended, because at 1) this should have be splatted to [-∞, ∞], which should have propegated through. This in theory could be fixed, but will need to be revisited for validation testing.

So this is the bit that I'm confused about. For f16, why didn't max + 2 cause a +∞ interval? I thought all greater-than-max-representable caused an inf?

@zoddicus
Copy link
Contributor Author

zoddicus commented Dec 6, 2023

Thank you for the example!

The framework for f16 currently returns [32752.0, 32768.0], which is in bounds. The current implementation isn't behaving as intended, because at 1) this should have be splatted to [-∞, ∞], which should have propegated through. This in theory could be fixed, but will need to be revisited for validation testing.

So this is the bit that I'm confused about. For f16, why didn't max + 2 cause a +∞ interval? I thought all greater-than-max-representable caused an inf?

Ugh, so my test case was wrong, it should have been plus 1 ULP(max/2), since for f32 and AF the values are large enough that plus 1 will just round to max/2. I had a typo in the my actual test code that was making f16 fail 🙃

So I cannot reproduce the issue I was seeing before anywhere, which makes me suspicious that it might have been a bug in something on my previous workstation...

I am going to port my mix patch to ToT and try determine if it is broken there or not.

@zoddicus
Copy link
Contributor Author

zoddicus commented Dec 7, 2023

Abandoning for now, since I don't think the rework is needed to get mix tests working atm. I may revisit this if OOB bit tracking it needed in the future.

@zoddicus zoddicus closed this Dec 7, 2023
@zoddicus zoddicus deleted the renameUnboundedInterval branch December 7, 2023 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants