Skip to content

GPU: give Metal its own spellings for two math helpers - #15932

Merged
ktf merged 1 commit into
AliceO2Group:devfrom
ktf:pr15932
Oct 11, 2026
Merged

ktf merged 1 commit into
AliceO2Group:devfrom
ktf:pr15932

Conversation

@ktf

@ktf ktf commented Oct 11, 2026

Copy link
Copy Markdown
Member

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.

@ktf
ktf requested a review from davidrohr as a code owner October 11, 2026 12:30
Comment thread GPU/Common/GPUCommonMath.h Outdated
}

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.
@ktf

ktf commented Oct 11, 2026

Copy link
Copy Markdown
Member Author

Already checked. Only change is the ifdef, as discussed. Merging.

@ktf
ktf merged commit 07e646c into AliceO2Group:dev Oct 11, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants