Drop the <fenv.h> dependency to stop leaking FE_* into includers - #774
Drop the <fenv.h> dependency to stop leaking FE_* into includers#774alanhc wants to merge 2 commits into
Conversation
Fixes DLTcollab#767. sse2neon.h includes <fenv.h>, so every translation unit that includes sse2neon.h also gets FE_INVALID, FE_TONEAREST and the rest. That breaks projects which deliberately define their own: RecoilEngine reports streflop emitting "FE_XXX flags were already defined and will be redefined" for every file, because streflop replaces the system FE_* macros on purpose. The include exists only for _MM_GET_ROUNDING_MODE and _MM_SET_ROUNDING_MODE, which is more indirection than the job needs. The rounding mode lives in FPCR.RMode, bits [23:22], and this header already has everything required to reach it: _sse2neon_get_fpcr()/_sse2neon_set_fpcr() and the fpcr_bitfield struct, whose bit22 and bit23 members are exactly that field. The sibling _MM_GET_FLUSH_ZERO_MODE and _MM_SET_FLUSH_ZERO_MODE already manipulate FPCR this way, so the two functions now follow the same shape. On AArch32 they use the same vmrs/vmsr FPSCR access as those siblings. Mapping, per the Arm ARM: 0b00 nearest, 0b01 toward +infinity, 0b10 toward -infinity, 0b11 toward zero. Invalid input to _MM_SET_ROUNDING_MODE still falls back to truncation, matching the previous behaviour. Besides fixing the reported problem this removes a libm dependency and replaces a libc call with a single MRS. Verified equivalent rather than assumed: a test that sets each mode through _MM_SET_ROUNDING_MODE and then reads fegetround() finds them in agreement for all four modes, on QEMU and on a Tensor G5 device, and 1.0f/3.0f differs between the round-down and round-up settings, so the FPCR write demonstrably takes effect. No new tests. The existing harness already covers this well: test_mm_set_rounding_mode checks that _mm_round_ps(_MM_FROUND_CUR_DIRECTION) agrees with the explicit-mode form in each of the four modes, and nine _mm_cvt* tests set rounding modes and validate their results. make CROSS_COMPILE=aarch64-linux-gnu- check passes 525/49/131/20 with zero failures, and again with STRICT_ALIASING=1. The AArch32 path was compile-checked and confirmed to emit vmrs/vmsr.
Performance Tier Documentation ReminderThe This is not an error - just a reminder to update the documentation if your changes affect intrinsic implementations. To regenerate the performance tier report: python3 scripts/gen-perf-report.py --clang-ast --weighted > perf-tier.mdFor detailed analysis: python3 scripts/analyze-tiers.py --clang-ast --weighted --markdown |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| #if SSE2NEON_ARCH_AARCH64 | ||
| _sse2neon_set_fpcr(r.value); | ||
| #else | ||
| __asm__ __volatile__("vmsr FPSCR, %0" ::"r"(r)); /* write */ | ||
| #endif |
There was a problem hiding this comment.
How about unifying _sse2neon_set_fpcr for both cases?
There was a problem hiding this comment.
Done in 9197ec2. It turned out the same #if was repeated at nine call sites across six functions, so I pushed the split down into the accessors and added a _sse2neon_fpcr_t typedef for the register width. That also collapsed the six copies of the union { fpcr_bitfield field; <width> value; } r; declaration, which each carried the same #if inside — net 41 lines removed.
I touched all six functions rather than only the two this PR added. Leaving _MM_GET/SET_FLUSH_ZERO_MODE and the denormals pair in the old shape would have made the file inconsistent with itself, but it does widen a bug fix into a small refactor.
The AArch32 write sites were passing the union rather than its value:
__asm__ __volatile__("vmsr FPSCR, %0" ::"r"(r)); /* r is the union */Going through the accessor passes r.value, so those three are gone. An AArch32 build now emits six vmrs and three vmsr, matching the six reads and three writes, and objdump shows the write reaching vmsr through a plain register (bfi r1, r0, #24, #1 then vmsr fpscr, r1). Same 34 pre-existing warnings as master, none added.
One caveat on the green CI: it was for 31c24f7. This commit changes the accessors themselves, including the _ReadStatusReg(ARM64_FPCR) path the MSVC jobs exercise, so those results do not carry over and the workflows need another approval to be meaningful.
Review feedback on DLTcollab#774. Every caller of the FPCR accessors carried its own #if SSE2NEON_ARCH_AARCH64 to pick between the helper and inline vmrs/vmsr, so the architecture split was repeated nine times across six functions. Push it down into the accessors instead. _sse2neon_get_fpcr and _sse2neon_set_fpcr now handle both execution states, and a _sse2neon_fpcr_t typedef carries the register width: uint64_t for FPCR, uint32_t for FPSCR. That also collapses the six copies of the union { fpcr_bitfield field; <width> value; } r; declaration, which each had the same #if inside. Net 41 lines removed. Touching all six functions rather than only the two this PR added is deliberate: leaving _MM_GET/SET_FLUSH_ZERO_MODE and the denormals pair with the old shape would have made the file inconsistent with itself. Say the word if you would rather have that split out. One incidental fix. The AArch32 write sites passed the union rather than its value: __asm__ __volatile__("vmsr FPSCR, %0" ::"r"(r)); Going through the accessor passes r.value, so the three of those are gone. Objdump of an AArch32 build confirms the write now reaches vmsr through a plain register (bfi r1, r0, DLTcollab#24, DLTcollab#1 ; vmsr fpscr, r1). Verified: aarch64 C and C++ clean under the project's warning flags; an AArch32 build emits six vmrs and three vmsr, exactly the six reads and three writes, with the same 34 pre-existing warnings as master and none added; the rounding equivalence test still agrees with fegetround in all four modes on QEMU and on a Tensor G5; check passes 525/49/131/20, also with STRICT_ALIASING=1; check-macros reports 0 errors and clang-format is a no-op.
|
I defer to @Cuda-Chen for confirmation. |
2 participants
Fixes #767.
sse2neon.h includes <fenv.h>, so every translation unit that includes sse2neon.h also gets FE_INVALID, FE_TONEAREST and the rest. That breaks projects which deliberately define their own: RecoilEngine reports streflop emitting "FE_XXX flags were already defined and will be redefined" for every file, because streflop replaces the system FE_* macros on purpose.
The include exists only for _MM_GET_ROUNDING_MODE and _MM_SET_ROUNDING_MODE, which is more indirection than the job needs. The rounding mode lives in FPCR.RMode, bits [23:22], and this header already has everything required to reach it: _sse2neon_get_fpcr()/_sse2neon_set_fpcr() and the fpcr_bitfield struct, whose bit22 and bit23 members are exactly that field. The sibling _MM_GET_FLUSH_ZERO_MODE and _MM_SET_FLUSH_ZERO_MODE already manipulate FPCR this way, so the two functions now follow the same shape. On AArch32 they use the same vmrs/vmsr FPSCR access as those siblings.
Mapping, per the Arm ARM: 0b00 nearest, 0b01 toward +infinity, 0b10 toward -infinity, 0b11 toward zero. Invalid input to _MM_SET_ROUNDING_MODE still falls back to truncation, matching the previous behaviour.
Besides fixing the reported problem this removes a libm dependency and replaces a libc call with a single MRS.
Verified equivalent rather than assumed: a test that sets each mode through _MM_SET_ROUNDING_MODE and then reads fegetround() finds them in agreement for all four modes, on QEMU and on a Tensor G5 device, and 1.0f/3.0f differs between the round-down and round-up settings, so the FPCR write demonstrably takes effect.
No new tests. The existing harness already covers this well: test_mm_set_rounding_mode checks that _mm_round_ps(_MM_FROUND_CUR_DIRECTION) agrees with the explicit-mode form in each of the four modes, and nine _mm_cvt* tests set rounding modes and validate their results. make CROSS_COMPILE=aarch64-linux-gnu- check passes 525/49/131/20 with zero failures, and again with STRICT_ALIASING=1. The AArch32 path was compile-checked and confirmed to emit vmrs/vmsr.
Summary by cubic
Removes the
<fenv.h>dependency fromsse2neon.hby reading/writing FPCR/FPSCR directly for rounding mode. This stops leakingFE_*into includers and fixes redefinition conflicts (Fixes #767) without changing observable behavior._MM_GET_ROUNDING_MODE/_MM_SET_ROUNDING_MODEnow map FPCR.RMode bits [23:22] (00 nearest, 01 up, 10 down, 11 toward zero); invalid input still truncates._sse2neon_get_fpcr/_sse2neon_set_fpcracross AArch64/AArch32 via a new_sse2neon_fpcr_t, removing per-callsite#ifand usingmrs/msrorvmrs/vmsras appropriate.fegetround/fesetroundand anylibm/libccalls; projects redefiningFE_*(e.g., streflop) no longer see warnings.Written for commit 9197ec2. Summary will update on new commits.