[libclc] Optimize generic CLC fmin/fmax #128506
Conversation
|
Note @arsenm I didn't touch amdgcn's fmin/fmax or r600's as I wasn't sure if any of that could be updated or at least unified. Would you be able to help there? I note that the comments around the use of canonicalize mention sNAN, which isn't required by the spec. |
|
These should use the regular builtin fmin / fmax.
The spec is quite badly written on what's expected of snans here, and the conformance test doesn't test what is written in the spec (hoping to fix that here |
Do you mean the AMD implementations, or the CLC ones too? Note there's no vector support for
Thanks for the link. I was going by 7.2 but now I see there's also a footnote. |
|
I don't suppose the recent clarifications to |
|
ping, thanks |
frasercrmck
force-pushed
the
libclc-clc-fmin-fmax
branch
from
43d4d7d to
572780f
Compare
This is an alternative to llvm#128506 which doesn't attempt to change the codegen for fmin and fmax on their way to the CLC library. The amdgcn and r600 custom definitions of fmin/fmax are now converted to custom definitions of __clc_fmin and __clc_fmax. The only codegen change is that non-standard vector/scalar overloads of fmin/fmax have been removed. We were currently (accidentally, presumably) providing overloads with mixed elment types such as fmin(double2, float), fmax(half4, double), etc. The only vector/scalar overloads in the OpenCL spec are those with scalars of the same element type as the vector in the first argument.
This is an alternative to #128506 which doesn't attempt to change the codegen for fmin and fmax on their way to the CLC library. The amdgcn and r600 custom definitions of fmin/fmax are now converted to custom definitions of __clc_fmin and __clc_fmax. For simplicity, the CLC library doesn't provide vector/scalar versions of these builtins. The OpenCL layer wraps those up to the vector/vector versions. The only codegen change is that non-standard vector/scalar overloads of fmin/fmax have been removed. We were currently (accidentally, presumably) providing overloads with mixed elment types such as fmin(double2, float), fmax(half4, double), etc. The only vector/scalar overloads in the OpenCL spec are those with scalars of the same element type as the vector in the first argument.
This is an alternative to llvm#128506 which doesn't attempt to change the codegen for fmin and fmax on their way to the CLC library. The amdgcn and r600 custom definitions of fmin/fmax are now converted to custom definitions of __clc_fmin and __clc_fmax. For simplicity, the CLC library doesn't provide vector/scalar versions of these builtins. The OpenCL layer wraps those up to the vector/vector versions. The only codegen change is that non-standard vector/scalar overloads of fmin/fmax have been removed. We were currently (accidentally, presumably) providing overloads with mixed elment types such as fmin(double2, float), fmax(half4, double), etc. The only vector/scalar overloads in the OpenCL spec are those with scalars of the same element type as the vector in the first argument.
frasercrmck
force-pushed
the
libclc-clc-fmin-fmax
branch
from
572780f to
5c367b8
Compare
frasercrmck
changed the title
It depends on whether the conformance test is fixed to match the fuzzy language of the spec or not. If the decision is fmin/fmax should match the IEEE behavior, the implementation directly maps to llvm.minnum/llvm.maxnum. If the decision is the conformance test continues doing what it has been doing, it should directly map to llvm.minimumnum/maximumnum. In either case, we should not have code using canonicalizes |
This is an alternative to llvm#128506 which doesn't attempt to change the codegen for fmin and fmax on their way to the CLC library. The amdgcn and r600 custom definitions of fmin/fmax are now converted to custom definitions of __clc_fmin and __clc_fmax. For simplicity, the CLC library doesn't provide vector/scalar versions of these builtins. The OpenCL layer wraps those up to the vector/vector versions. The only codegen change is that non-standard vector/scalar overloads of fmin/fmax have been removed. We were currently (accidentally, presumably) providing overloads with mixed elment types such as fmin(double2, float), fmax(half4, double), etc. The only vector/scalar overloads in the OpenCL spec are those with scalars of the same element type as the vector in the first argument.
This is an alternative to llvm#128506 which doesn't attempt to change the codegen for fmin and fmax on their way to the CLC library. The amdgcn and r600 custom definitions of fmin/fmax are now converted to custom definitions of __clc_fmin and __clc_fmax. For simplicity, the CLC library doesn't provide vector/scalar versions of these builtins. The OpenCL layer wraps those up to the vector/vector versions. The only codegen change is that non-standard vector/scalar overloads of fmin/fmax have been removed. We were currently (accidentally, presumably) providing overloads with mixed elment types such as fmin(double2, float), fmax(half4, double), etc. The only vector/scalar overloads in the OpenCL spec are those with scalars of the same element type as the vector in the first argument.
This is an alternative to llvm#128506 which doesn't attempt to change the codegen for fmin and fmax on their way to the CLC library. The amdgcn and r600 custom definitions of fmin/fmax are now converted to custom definitions of __clc_fmin and __clc_fmax. For simplicity, the CLC library doesn't provide vector/scalar versions of these builtins. The OpenCL layer wraps those up to the vector/vector versions. The only codegen change is that non-standard vector/scalar overloads of fmin/fmax have been removed. We were currently (accidentally, presumably) providing overloads with mixed elment types such as fmin(double2, float), fmax(half4, double), etc. The only vector/scalar overloads in the OpenCL spec are those with scalars of the same element type as the vector in the first argument.
This is an alternative to llvm#128506 which doesn't attempt to change the codegen for fmin and fmax on their way to the CLC library. The amdgcn and r600 custom definitions of fmin/fmax are now converted to custom definitions of __clc_fmin and __clc_fmax. For simplicity, the CLC library doesn't provide vector/scalar versions of these builtins. The OpenCL layer wraps those up to the vector/vector versions. The only codegen change is that non-standard vector/scalar overloads of fmin/fmax have been removed. We were currently (accidentally, presumably) providing overloads with mixed elment types such as fmin(double2, float), fmax(half4, double), etc. The only vector/scalar overloads in the OpenCL spec are those with scalars of the same element type as the vector in the first argument.
This is an alternative to llvm#128506 which doesn't attempt to change the codegen for fmin and fmax on their way to the CLC library. The amdgcn and r600 custom definitions of fmin/fmax are now converted to custom definitions of __clc_fmin and __clc_fmax. For simplicity, the CLC library doesn't provide vector/scalar versions of these builtins. The OpenCL layer wraps those up to the vector/vector versions. The only codegen change is that non-standard vector/scalar overloads of fmin/fmax have been removed. We were currently (accidentally, presumably) providing overloads with mixed elment types such as fmin(double2, float), fmax(half4, double), etc. The only vector/scalar overloads in the OpenCL spec are those with scalars of the same element type as the vector in the first argument.
For now, @frasercrmck can we update this PR to use __builtin_elementwise_maximumnum/minimumnum so that OpenCL CTS can pass? |
…#149775) Addresses llvm#112164. minimumnum and maximumnum intrinsics were added in 5bf81e5. The new built-ins can be used for implementing OpenCL math function fmax and fmin in llvm#128506.
The CLC fmin/fmax builtins now use clang's __builtin_elementwise_(min|max) which helps us generate llvm.(min|max)num intrinsics directly. These intrinsics select the non-NAN input over the NAN input, which adheres to the OpenCL specification. Note that the OpenCL specification doesn't require support for sNAN, so returning qNAN over sNAN is acceptable. Note also that the intrinsics don't differentiate between -0.0 and +0.0; this does not appear to be required - going by the OpenCL CTS, at least. These intrinsics maintain the vector types, as opposed to scalarizing, which was previously happening. This commit therefore helps to optimize codegen for those targets.
frasercrmck
force-pushed
the
libclc-clc-fmin-fmax
branch
from
5c367b8 to
8cd4a8e
Compare
Good idea, I've done that now. The amdgcn/r600 versions now use One caveat is that I had to make the SPIR-V targets use the old versions of CLC fmin/fmax as the Khronos SPIR-V translator can't (currently?) handle those intrinsics. |
With this commit, the CLC fmin/fmax builtins use clang's _builtin_elementwise(min|max)imumnum which helps us generate LLVM minimumnum/maximumnum intrinsics directly. These intrinsics uniformly select the non-NaN input over the (quiet or signalling) NaN input, which corresponds to what the OpenCL CTS tests.
These intrinsics maintain the vector types, as opposed to scalarizing, which was previously happening. This commit therefore helps to optimize codegen for those targets.
Note that there is ongoing discussion regarding how these builtins should handle signalling NaNs in the OpenCL specification and whether they should be able to return a quiet NaN as per the IEEE behaviour. If the specification and/or CTS is ever updated to allow or mandate returning a qNAN, these builtins could/should be updated to use _builtin_elementwise(min|max)num instead which would lower to LLVM minnum/maxnum intrinsics.
The SPIR-V targets maintain the old implementations, as the LLVM -> SPIR-V translator can't currently handle the LLVM intrinsics. The implementation has been simplifies to consistently use clang builtins, as opposed to before where the half version was explicitly defined.
[1] KhronosGroup/OpenCL-CTS#2285