Skip to content

[UnitTest] Add test for fmax reductions without fast-math. #266

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

Merged
merged 4 commits into from
Jul 18, 2025

Conversation

fhahn
Copy link
Contributor

@fhahn fhahn commented Jul 17, 2025

Add unit tests for vectorizing FMax reductions without fast-math flags, covering a wide range of inputs, including various combinations of NaNs and signed-zeros.

Add unit tests for vectorizing FMax reductions without fast-math flags,
covering a wide range of inputs, including various combinations of NaNs
and signed-zeros.
check(ScalarFn, VectorFn, &Src1[0], N, "full-with-multiple-nan");
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to test signaling nans, but it will be busted all over the place

Test some denormals?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some tests with denormals both as start value and in the inputs, thanks

At least for AArch64, the tests also pass when replacing all quiet nans with signaling nans with llvm/llvm-project#148239, as it just checks if it matches the behavior of the scalar loop.

But in general it may be a bit risky to check signaling NaNs, as the behavior may not be 100% consistent across platforms?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be, but the state is in flux and has never been consistent

Comment on lines 11 to 14
if (std::isnan(A))
return std::isnan(B);

if (A == 0.0f)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if isnan(B) and A == 0.0f this becomes dependent on the nan signbit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the check above to check if either is nan, in that case both must be nan to be equal. I also check here if B is zero. I think that should take care of that.

@fhahn fhahn merged commit 42dab80 into llvm:main Jul 18, 2025
1 check passed
@fhahn fhahn deleted the vector-fmax-without-fast-math branch July 18, 2025 14:23
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.

2 participants