Skip to content

Ignore sign difference between two NaNs #1155

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

Closed
wants to merge 3 commits into from

Conversation

dtcxzyw
Copy link
Contributor

@dtcxzyw dtcxzyw commented Jan 4, 2025

As we discussed in #1037, we don't care about the sign of NaN values. This patch only checks the NaN payload when both values are NaNs.

Here is a false positive case: https://alive2.llvm.org/ce/z/iSh4-Z
Related PR: llvm/llvm-project#121580

@dtcxzyw dtcxzyw marked this pull request as draft January 5, 2025 07:27
@dtcxzyw
Copy link
Contributor Author

dtcxzyw commented Jan 5, 2025

I don't know how to fix the test failure in tests/alive-tv/bugs/pr45152.srctgt.ll :(

@dtcxzyw dtcxzyw marked this pull request as ready for review January 5, 2025 13:23
@nunoplopes
Copy link
Member

There was no discussion about this change of the semantics AFAICT. Could you please open a discuss first at discourse so we can reach consensus before changing Alive2?
Thanks!

@dtcxzyw
Copy link
Contributor Author

dtcxzyw commented Apr 22, 2025

I decided to fix it in LLVM. This case is a little different from #1037 since there is no floating-point operation being performed. As per the IEEE 754-2008 standard, the sign bit of a NaN is only allowed to be refined in floating point operations except for copy, negate, abs, and copySign.

@dtcxzyw dtcxzyw closed this Apr 22, 2025
@dtcxzyw
Copy link
Contributor Author

dtcxzyw commented Apr 22, 2025

See also llvm/llvm-project#59279

@regehr
Copy link
Contributor

regehr commented Apr 22, 2025

sorry to add to a closed issue, but I just want to add that using arm-tv I run across quite a lot of issues relating to LLVM and Alive disagreeing about whether NaN and +0/-1 transformations are valid. @dtcxzyw I would be super happy to feed you examples if you're interested!

@dtcxzyw
Copy link
Contributor Author

dtcxzyw commented Apr 22, 2025

sorry to add to a closed issue, but I just want to add that using arm-tv I run across quite a lot of issues relating to LLVM and Alive disagreeing about whether NaN and +0/-1 transformations are valid. @dtcxzyw I would be super happy to feed you examples if you're interested!

Feel free to create a post on Discourse, and then we can discuss these cases in public :)
I also found some disagreements between Alive2 and InstCombine with mutation-based fuzzing. I am drafting two RFCs to clarify the LangRef wording.

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.

3 participants