[libclc] Refine __clc_fp*_subnormals_supported#157633
[libclc] Refine __clc_fp*_subnormals_supported#157633
Conversation
…al_if_not_supported Remove the dependency on the libclc build-time configuration for __clc_fp*_subnormals_supported. The check is now implemented with LLVM intrinsics so it can be resolved during target lowering or at runtime. Improve __clc_flush_denormal_if_not_supported implementation as well. It doesn't use __clc_fp*_subnormals_supported which canonicalizes sNaN and thus the new implementation is more foldable. Remove cmake option ENABLE_RUNTIME_SUBNORMAL and related code. Resolves llvm#153148 Co-authored-by: Matt Arsenault <Matthew.Arsenault@amd.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the subnormal support detection and handling in libclc by replacing build-time configuration with runtime intrinsic-based checks. The implementation now uses LLVM's __builtin_canonicalizef and __builtin_isfpclass intrinsics to determine subnormal support at runtime rather than relying on cmake configuration options.
Key changes:
- Replaces build-time subnormal configuration with runtime intrinsic-based detection
- Removes cmake option
ENABLE_RUNTIME_SUBNORMALand related build logic - Improves
__clc_flush_denormal_if_not_supportedimplementation to be more optimizable
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
libclc/clc/lib/generic/math/clc_subnormal_config.cl |
New implementation using LLVM intrinsics for runtime subnormal detection |
libclc/clc/include/clc/math/clc_subnormal_config.h |
Removes unused function declaration |
libclc/clc/include/clc/math/math.h |
Refactors flush denormal function and removes header dependency |
| Multiple math files | Removes unnecessary include of subnormal config header |
| Multiple SOURCES files | Removes subnormal config files from build lists |
libclc/CMakeLists.txt |
Removes cmake option and related build configuration |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Thanks, I forgot that part. Done in 4608f77 |
libclc/clc/include/clc/math/math.h
Outdated
| // Avoid calling __clc_fp32_subnormals_supported here: it uses | ||
| // llvm.canonicalize, which quiets sNaN. | ||
| return __builtin_fabsf(x) < 0x1p-149f | ||
| ? __builtin_elementwise_copysign(0.0f, x) | ||
| : x; |
There was a problem hiding this comment.
This function no longer matches the name or behavior. This is an unconditional flush to zero
There was a problem hiding this comment.
If denormal is not supported
- returns signed zero for denormal input since __builtin_fabsf result is 0
- return x if x is not denormal
If denormal is supported
- returns x as is regardless if x is denormal or not.
So this function only flushes to zero if denormal is not supported, right?
There was a problem hiding this comment.
There's no support check here. __builtin_fabsf(x) < 0x1p-149f will be true for 0 or a denormal value, regardless of whether the fcmp flushes the input
Also you can use __builtin_elementwise_abs to be consistently using elementwise builtins
There was a problem hiding this comment.
There's no support check here.
__builtin_fabsf(x) < 0x1p-149fwill be true for 0 or a denormal value, regardless of whether the fcmp flushes the inputAlso you can use __builtin_elementwise_abs to be consistently using elementwise builtins
thanks @arsenm. Now I see what you mean. Renamed __clc_flush_denormal_if_not_supported to __clc_soft_flush_denormal in 3f665ce and use __builtin_elementwise_abs
There was a problem hiding this comment.
The name is now right, but the usage is not. You do not want to unconditionally force a denormal flush, pretty much anywhere.
I also believe most of the explicit flushes / canonicalizes in the math function implementations can be deleted with some shuffling around. I removed most of these in the rocm-device-libs implementations a few years ago. A few more remain but I believe they can also be avoided
| a = __clc_soft_flush_denormal(a); | ||
| b = __clc_soft_flush_denormal(b); | ||
| c = __clc_soft_flush_denormal(c); |
There was a problem hiding this comment.
Unconditionally forcing flush of denormals is not desirable. In this context I'm not sure why it's trying to flush in the first place.
The below code extracting the exponent can be replaced with frexp, and the return c on the above paths is missing a canonicalize.
But on a deeper level I don't think libclc should be trying to provide a software FMA implementation in the first place; that's a decision for the compiler when codegening llvm.fma, surely compiler-rt already has an implementation?
There was a problem hiding this comment.
Unconditionally forcing flush of denormals is not desirable. In this context I'm not sure why it's trying to flush in the first place.
The below code extracting the exponent can be replaced with frexp, and the return c on the above paths is missing a canonicalize.
But on a deeper level I don't think libclc should be trying to provide a software FMA implementation in the first place; that's a decision for the compiler when codegening llvm.fma, surely compiler-rt already has an implementation?
Deleted clc_sw_fma in 7b290a2
Now clc_fma is implemented with __builtin_elementwise_fma
There was a problem hiding this comment.
surely compiler-rt already has an implementation?
It doesn't. LLVM libc has one but it uses FP64, so I don't think it is of much help. I'd expect most targets that don't have hardware fma don't have fp64 either.
I think dropping sw fma would impact:
- SPIR-V, which then starts generating the
GLS.std.450extended instruction FMA. The problem there is that instruction is (AFAICT) allowed to round intermediate products, but the OpenCL spec doesn't allow that. I'm not sure if drivers actually implement it as fused or not.
Arguably the lowering@llvm.fmato this instruction is bug in LLVM as@llvm.fmais specified to be fused without fast math flags. - Not all old R600 targets have FMA, I think this change would be breaking them. These are >10 years old GPUs at this point though.
There was a problem hiding this comment.
I think r600 always had FMA, it's just not "fast" on all of them. In any case, the backed is obligated to implement llvm.fma correctly
There was a problem hiding this comment.
SPIR-V don't need __clc_sw_fma anymore if SPV_KHR_fma extension is enabled. The extension is already implemented in llvm-spirv and soon will be implemented in llvm SPIR-V backend: #173057
arsenm
left a comment
There was a problem hiding this comment.
I think dropping the soft FMA is beyond the scope of this patch, but it is something I think should be done
| #pragma OPENCL EXTENSION cl_khr_fp64 : enable | ||
| _CLC_DEF bool __clc_fp64_subnormals_supported() { | ||
| #ifdef CLC_SPIRV | ||
| // SPIR-V doesn't support llvm.canonicalize for now. |
There was a problem hiding this comment.
Can you just fix that instead of special casing it here? It's not difficult to implement
There was a problem hiding this comment.
Can you just fix that instead of special casing it here? It's not difficult to implement
done in ccf7a6e
| x = __clc_soft_flush_denormal(x); | ||
| y = __clc_soft_flush_denormal(y); |
There was a problem hiding this comment.
I don't think this is necessary. In the rocm-device-libs version of this, I managed to delete the explicit canonicalizes
There was a problem hiding this comment.
See this series of patches:
ROCm@b3beb93
ROCm@9a7bc19
ROCm@e9198f7
Should just copy what these did
There was a problem hiding this comment.
See this series of patches: ROCm@b3beb93 ROCm@9a7bc19 ROCm@e9198f7
Should just copy what these did
I have tried to port both libclc __clc_remquo and ocml remquo2 to replace intel gpu implementation at https://github.com/intel/intel-graphics-compiler/blob/fc97dc482697b320667a52914f1225556f0856e8/IGC/BiFModule/Implementation/Math/remquo.cl#L12-L104, however, the ported code can't pass OpenCL CTS test ./test_bruteforce remquo on intel gpu.
Can I copy intel gpu implementation to overwrite libclc __clc_remquo?
libclc/clc/include/clc/math/math.h
Outdated
| // Avoid calling __clc_fp32_subnormals_supported here: it uses | ||
| // llvm.canonicalize, which quiets sNaN. | ||
| return __builtin_fabsf(x) < 0x1p-149f | ||
| ? __builtin_elementwise_copysign(0.0f, x) | ||
| : x; |
There was a problem hiding this comment.
The name is now right, but the usage is not. You do not want to unconditionally force a denormal flush, pretty much anywhere.
I also believe most of the explicit flushes / canonicalizes in the math function implementations can be deleted with some shuffling around. I removed most of these in the rocm-device-libs implementations a few years ago. A few more remain but I believe they can also be avoided
This reverts commit 7b290a2.
…ng llvm.canonicalize
libclc/clc/include/clc/math/math.h
Outdated
| } | ||
| return x; | ||
| _CLC_OVERLOAD _CLC_INLINE float __clc_soft_flush_denormal(float x) { | ||
| // Avoid calling __clc_fp32_subnormals_supported here: it uses |
There was a problem hiding this comment.
You might have less trouble just using canonicalize for now and trying to relax it later
There was a problem hiding this comment.
You might have less trouble just using canonicalize for now and trying to relax it later
do you mean reverting __clc_soft_flush_denormal to use __clc_fp32_subnormals_supported which uses llvm.canonicalize, or just replacing use of __clc_fp32_subnormals_supported with __builtin_elementwise_canonicalize?
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
restored clc_sw_fma in b61e32b |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static _CLC_INLINE float __clc_flush_denormal_if_not_supported(float x) { | ||
| int ix = __clc_as_int(x); | ||
| if (!__clc_fp32_subnormals_supported() && ((ix & EXPBITS_SP32) == 0) && | ||
| ((ix & MANTBITS_SP32) != 0)) { | ||
| ix &= SIGNBIT_SP32; | ||
| x = __clc_as_float(ix); | ||
| } | ||
| return x; | ||
| } |
There was a problem hiding this comment.
This shouldn't do all of this bithacking. This is canonicalize, which at worst costs an extra instruction you might not have needed. With the pattern from #172998, you can have this conditional flush that doesn't have the cost of signaling nan quieting and can fold away without DAZ
There was a problem hiding this comment.
changed to use snan check and __builtin_elementwise_canonicalize in 5eb6990.
Please review if it meets the expectation.
| // SPIR-V doesn't support llvm.canonicalize. Synthesize a subnormal by halving | ||
| // the smallest normal. If subnormals are not supported it will flush to +0. | ||
| float smallest_normal = 0x1p-126f; | ||
| float sub = |
There was a problem hiding this comment.
Don't need multiply, and I think this logic should be inverted. Denormal support is the base case, DAZ is an aberration. i.e., this is what I did in device-libs is about
is_daz() { return __builtin_isfpclass(__builtin_canonicalizef(0x1p-149f), __FPCLASS_POSZERO) }
| #ifdef cl_khr_fp64 | ||
| #pragma OPENCL EXTENSION cl_khr_fp64 : enable | ||
| _CLC_DEF bool __clc_fp64_subnormals_supported() { | ||
| // SPIR-V doesn't support llvm.canonicalize. Synthesize a subnormal by halving |
There was a problem hiding this comment.
Fix SPIRV, this is not a reasonable workaround. This is not a difficult to implement intrinsic
There was a problem hiding this comment.
changed SPIRV path to use __clc_fabs(0x1p-149f) in 7ff4dcf
There was a problem hiding this comment.
I've made an attempt to add the lowering in #178439
This PR is extracted from llvm#157633. `-fdenormal-fp-math=dynamic` is required to defer denormal handling and should be used for libclc library compilation. Additionally, if the default ieee value is incompatible with the user code's denormal-fp-math setting, this mismatch prevents libclc functions from being inlined.
This PR is extracted from #157633. `-fdenormal-fp-math=dynamic` is required to defer denormal handling and should be used for libclc library compilation. Additionally, if the default ieee value is incompatible with the user code's denormal-fp-math setting, this mismatch prevents libclc functions from being inlined.
Remove the dependency on the libclc build-time configuration for __clc_fp*_subnormals_supported. The check is now implemented with LLVM intrinsics so it can be resolved during target lowering or at runtime.
-fdenormal-fp-math=dynamic build flag is required to defer denormal handling.
Remove cmake option ENABLE_RUNTIME_SUBNORMAL and related code.
Resolves #153148