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

[PPC] relax fastmath test #38850

Merged
merged 1 commit into from
Dec 14, 2020
Merged

[PPC] relax fastmath test #38850

merged 1 commit into from
Dec 14, 2020

Conversation

vchuravy
Copy link
Member

@shirodkara if I recall our discussion correctly the backend stance was there is no guarantee that
this operations yield Inf under fastmath?

@vchuravy vchuravy added the system:powerpc PowerPC label Dec 12, 2020
@vtjnash
Copy link
Member

vtjnash commented Dec 12, 2020

I'm not sure of the purpose of any of these tests. Isn't this undefined behavior for "fast" math and the program is allowed to do anything when encountering a NaN or Inf value.

@vchuravy
Copy link
Member Author

I'm not sure of the purpose of any of these tests. Isn't this undefined behavior for "fast" math and the program is allowed to do anything when encountering a NaN or Inf value.

I was wondering about that as well. These were originally added in #9759 by @eschnett.

@eschnett
Copy link
Contributor

Yes, these tests are over-zealous. As @vtjnash suggests, they only test incidental behaviour. They could be replace by test that merely ensure that these tests do not throw an exception.

@vchuravy vchuravy requested a review from eschnett December 12, 2020 20:50
Copy link
Contributor

@eschnett eschnett left a comment

Choose a reason for hiding this comment

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

You could say isa T instead of != one.

@vtjnash
Copy link
Member

vtjnash commented Dec 13, 2020

There’s no guarantee this doesn’t throw, or other much worse things.

@eschnett
Copy link
Contributor

I see that LLVM's definition for the fast-math behaviour has changed over time. It used to guarantee that the resulting behaviour is an undefined value, but won't throw. This is now different – the fast-math flags seem to require that nans and infs are detected and produce a poison value? I'm not familiar enough with poison values to see how this could be implemented efficiently.

@vchuravy
Copy link
Member Author

@eschnett a poison value is like undef but with propagation semantics. I collected a couple of references here #38735 (comment)

With regard to this PR. Should I remove the tests since the explicitly result in undef/poison?

@eschnett
Copy link
Contributor

@vchuravy If you confirm that these tests should not be there (according to https://llvm.org/docs/LangRef.html#fast-math-flags), then yes, please remove them.

@vchuravy
Copy link
Member Author

Yes that is my reading of the spec.

@eschnett
Copy link
Contributor

@vchuravy I would just remove these lines.

@vchuravy vchuravy added the backport 1.6 Change should be backported to release-1.6 label Dec 14, 2020
@vchuravy vchuravy merged commit b6c09a3 into master Dec 14, 2020
@vchuravy vchuravy deleted the vc/ppc_fastmath branch December 14, 2020 19:46
@KristofferC KristofferC mentioned this pull request Dec 17, 2020
53 tasks
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Dec 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants