Skip to content

Conversation

@Hamaguri-0414
Copy link
Contributor

We found a problem with audio sample code that uses changing index numbers for array access. This prevents the compiler from optimizing the code well, causing the shader to fail on older Metal GPUs (before A14 Bionic).
This fix limits the array access range clearly, which helps the compiler make better optimizations.

We tested the fix and confirmed the shader now works properly on Metal GPUs older than A14 Bionic.

@float3 float3 self-requested a review May 22, 2025 12:33
@float3
Copy link
Collaborator

float3 commented May 22, 2025

lgtm

@pema99 pema99 self-requested a review May 22, 2025 19:53
@pema99
Copy link
Collaborator

pema99 commented May 22, 2025

This really doesn't sit well with me. It's a de-optimization in almost all cases. Integer division and modulo are some of the absolute slowest operations you can do on a modern GPU. On top of that, the change increase the instruction count by around 7% and the register count by 50%. The code as it is now is pretty objectively better to me.

You write

This prevents the compiler from optimizing the code well, causing the shader to fail on older Metal GPUs (before A14 Bionic).

It's unclear to me not "optimizing the code well" translates into the shader failing. Do you actually get a compilation error? If so, which one? What if you slap a [branch] attribute on each of the if-statements? And what about with just adding the clamp?

Also p.s, your bound check is wrong. When passing index 4092, the check 4092 < sample will return false, and you will index out of bounds. Should be <=.

Copy link
Collaborator

@pema99 pema99 left a comment

Choose a reason for hiding this comment

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

I'd like to understand the issue you are addressing better before we consider merging this. If we do end up merging it, it should probably be behind an #ifdef for Metal only. Check my previous comment.

@pema99 pema99 marked this pull request as draft July 8, 2025 19:27
@pema99
Copy link
Collaborator

pema99 commented Jul 8, 2025

Marking as draft as this hasn't had any activity for quite a while.

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.

3 participants