Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix float vector comparisons with signed zero and NaN, add test coverage #21933

Merged
merged 4 commits into from
Mar 9, 2025

Conversation

kcbanner
Copy link
Contributor

@kcbanner kcbanner commented Nov 7, 2024

Closes #21932

  • Value: fix comparison of NaN in compareHeteroAdvanaced
  • Sema: fix equality comparison of signed zeroes and NaN in compareScalar
  • tests: add test coverage for vector float comparisons

@kcbanner
Copy link
Contributor Author

kcbanner commented Nov 7, 2024

I observed these failures on the CI, so I've disabled these new tests on these targets:

     +- run test behavior-arm-linux.4.19...6.11.5-eabihf-baseline-Debug 1871/1977 passed, 1 failed, 105 skipped
     +- run test behavior-arm-linux.4.19...6.11.5-musleabihf-baseline-Debug-libc 1871/1977 passed, 1 failed, 105 skipped
     +- run test behavior-thumb-linux.4.19...6.11.5-eabihf-baseline-Debug 1868/1977 passed, 1 failed, 108 skipped
     +- run test behavior-thumb-linux.4.19...6.11.5-musleabihf-baseline-Debug-libc 1868/1977 passed, 1 failed, 108 skipped
     +- run test behavior-powerpc64-linux.4.19...6.11.5-none-ppc64-Debug 1863/1977 passed, 1 failed, 113 skipped
     +- run test behavior-powerpc64-linux.4.19...6.11.5-musl-ppc64-Debug-libc 1863/1977 passed, 1 failed, 113 skipped
     +- run test behavior-powerpc64le-linux.4.19...6.11.5-gnu.2.28-ppc64le-Debug-libc
         +- zig test Debug powerpc64le-linux-gnu failure
error: Assertion failed: N->getValueType(0) == MVT::v1i1 && "Expected v1i1 type" (/home/andy/dev/zig-bootstrap/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp: ScalarizeVecOp_VSETCC: 913)

@kcbanner kcbanner force-pushed the comptime_nan_comparison branch from d34b378 to 8006052 Compare November 11, 2024 18:31
@alexrp alexrp requested a review from mlugg January 29, 2025 08:05
@alexrp alexrp added this to the 0.14.0 milestone Jan 30, 2025
@jacobly0
Copy link
Member

jacobly0 commented Feb 10, 2025

#22744 and maybe #22636

@andrewrk andrewrk force-pushed the comptime_nan_comparison branch from 8006052 to cae5790 Compare February 23, 2025 02:30
@andrewrk
Copy link
Member

Rebased and force-pushed. The only missing consideration I see here has to do with comptime function call memoization.

comptime call memoization should only occur if the float values are bit-for-bit equal, since floats have well-defined memory layout.

I'm unsure whether this changeset respects that rule or not, and unsure whether there is already behavior test coverage for this or not.

@alexrp
Copy link
Member

alexrp commented Feb 27, 2025

Breakage from changes in #22589: /home/ci/actions-runner9/_work/zig/zig/test/behavior/floatop.zig:147:52: error: no field or member function named 'floatAbi' in 'Target'

@kcbanner
Copy link
Contributor Author

I'll update this PR this weekend - I'm not familiar with how to test for memoization behaviour but I assume there are existing tests I can reference for that.

@andrewrk andrewrk removed this from the 0.14.0 milestone Mar 1, 2025
@kcbanner kcbanner force-pushed the comptime_nan_comparison branch from cae5790 to 144d69b Compare March 5, 2025 04:31
@kcbanner
Copy link
Contributor Author

kcbanner commented Mar 5, 2025

Fixed up the breakage from #22589 and added comptime memoization tests.

@andrewrk
Copy link
Member

andrewrk commented Mar 9, 2025

Thanks!

@andrewrk andrewrk merged commit 539f3ef into ziglang:master Mar 9, 2025
9 checks passed
andrewrk added a commit that referenced this pull request Mar 9, 2025
Fix float vector comparisons with signed zero and NaN, add test coverage
alichraghi pushed a commit to alichraghi/zig that referenced this pull request Mar 10, 2025
Fix float vector comparisons with signed zero and NaN, add test coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler panic caused by comparing a float vector that contains NaN
4 participants