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

Fix GPUAggregator on M chips #9128

Merged
merged 2 commits into from
Sep 3, 2024
Merged

Fix GPUAggregator on M chips #9128

merged 2 commits into from
Sep 3, 2024

Conversation

Pessimistress
Copy link
Collaborator

For #8887 (comment)

Tested on M2. Thanks @felixpalmer for the insight.

Change List

  • Avoid interpolating NaNs

@coveralls
Copy link

coveralls commented Sep 1, 2024

Coverage Status

coverage: 91.679%. remained the same
when pulling 6a60447 on x/gpu-aggregation-fix
into 1f1b155 on master.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Great to finally see a fix! I added MacOS if statements around some of the test cases in webgl-aggregator-spec.ts, you might want to remove those.


Thoughts:

Since I haven't dug into the problem, several questions come to mind.

  • Why do the 3 vertices have different values (leading to interpolation issues) in the first place ?
  • Because AFAIK, using flat basically means just pick the value from the first vertex and ignore the other two...
  • If the other two weren't needed, couldn't we just have populated them with the right values instead?
  • Is this a solution we expect to apply in more places?

@Pessimistress
Copy link
Collaborator Author

There was never interpolation involved. This pass is drawing points geometry.

@ibgreen
Copy link
Collaborator

ibgreen commented Sep 1, 2024

There was never interpolation involved. This pass is drawing points geometry.

Then why does disabling interpolation (i.e. using flat even make a difference? It affects some edge case around NaN handling?

@Pessimistress
Copy link
Collaborator Author

It was not an improperly written shader to begin with, just that it was executed in an unexpected way by the M GPU. Adding an explicit flat seems to prompt it to run correctly.

@ibgreen
Copy link
Collaborator

ibgreen commented Sep 1, 2024

OK sure. I did see that there was a linked discussion about shaders not being happy with NaNs. NaNs are notoriously unreliable.

My thinking is basically that if the "undefined" behavior of NaNs is really the root cause, might we want to consider avoiding NaNs instead of adding "black magic" GLSL qualifiers.

To be clear, I have not objections to landing this, just trying to see if we can deepen our understanding of the problem.

Copy link
Collaborator

@felixpalmer felixpalmer left a comment

Choose a reason for hiding this comment

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

I know it is the 4.5 spec, not 3.0 but I found this note on page 49:

Fragment shader inputs that are, or contain, signed or unsigned integers, integer vectors, or any double-
precision floating-point type must be qualified with the interpolation qualifier flat.

Perhaps this is what is happening under the covers, the precision loss is degrading the nan value.

At any rate, happy to report the fix works on M3 also

@ibgreen
Copy link
Collaborator

ibgreen commented Sep 2, 2024

I found this note on page 49:

Interesting. Sounds like best practice to add flat whenever we use fragment shaders for "pure calculations" then.

@Pessimistress Pessimistress merged commit 9a44132 into master Sep 3, 2024
4 checks passed
@Pessimistress Pessimistress deleted the x/gpu-aggregation-fix branch September 3, 2024 08:05
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.

4 participants