Repository navigation
GPU: give Metal its own spellings for two math helpers - #15932
Merged
Merged
Conversation
davidrohr
reviewed
Oct 11, 2026
| } | ||
|
|
||
| GPUdi() constexpr float GPUCommonMath::Modf(float x, float y) { return GPUCA_CHOICE(fmodf(x, y), fmodf(x, y), fmod(x, y)); } | ||
| #ifdef __METAL__ // MSL has no remainder(), so reduce with fmod, which is exact |
Collaborator
There was a problem hiding this comment.
Very minor comment: Why don't you put the #ifdef inside the function body, to reduce the duplicated code a bit? Besides, I am fine with it, you can merge it, or fix it and then merge.
Member
Author
There was a problem hiding this comment.
No particular reason. It was done by my friends, but frankly I would have done the same myself.
GPUCA_CHOICE routes Metal down the OpenCL arm, and three of those spellings do not exist in MSL. One of the three needs no Metal case: MSL has no nan() in any form, but it does define NAN, and so does OpenCL -- a constant expression of type float representing a quiet NaN -- so the shared arm uses that instead of nan(0u). remainder() does not exist either, and __builtin_remainderf is not a way around it: that compiles, then fails to link against an undefined remainderf. MSL does have fmod, which is exact, so Remainderf reduces with that and nudges the result into [-|y|/2, |y|/2], rounding the quotient of a tie to even. It agrees with remainderf bit for bit over six million samples, from the TwoPI wrapping its only caller does up to |x| of 1e30, ties swept exhaustively for a power-of-two divisor. Reducing as x - y * rint(x / y) would have been shorter, but x / y rounds in float, so rint picks the wrong multiple outside a narrow range around the divisor and the result is wrong almost everywhere else. MSL's sincos returns the sine and takes the cosine by thread reference rather than by pointer, and cannot write through the generic reference SinCos is given, so the result goes via a local. Host, CUDA, HIP and cling keep the GPUCA_CHOICE arms they had; OpenCL swaps nan(0u) for NAN.
Member
Author
|
Already checked. Only change is the ifdef, as discussed. Merging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GPUCA_CHOICE routes Metal down the OpenCL arm, and three of those spellings do
not exist in MSL. One of the three needs no Metal case: MSL has no nan() in any
form, but it does define NAN, and so does OpenCL -- a constant expression of
type float representing a quiet NaN -- so the shared arm uses that instead of
nan(0u).
remainder() does not exist either, and __builtin_remainderf is not a way around
it: that compiles, then fails to link against an undefined remainderf. MSL does
have fmod, which is exact, so Remainderf reduces with that and nudges the result
into [-|y|/2, |y|/2], rounding the quotient of a tie to even. It agrees with
remainderf bit for bit over six million samples, from the TwoPI wrapping its
only caller does up to |x| of 1e30, ties swept exhaustively for a power-of-two
divisor. Reducing as x - y * rint(x / y) would have been shorter, but x / y
rounds in float, so rint picks the wrong multiple outside a narrow range around
the divisor and the result is wrong almost everywhere else.
MSL's sincos returns the sine and takes the cosine by thread reference rather
than by pointer, and cannot write through the generic reference SinCos is given,
so the result goes via a local.
Host, CUDA, HIP and cling keep the GPUCA_CHOICE arms they had; OpenCL swaps
nan(0u) for NAN.