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

Deprecate MXCSR-related intrinsics #817

Closed
wants to merge 4 commits into from
Closed

Deprecate MXCSR-related intrinsics #817

wants to merge 4 commits into from

Conversation

GabrielMajeri
Copy link
Contributor

@GabrielMajeri GabrielMajeri commented Sep 30, 2019

Fixes #781.

@rust-highfive
Copy link

r? @gnzlbg

(rust_highfive has picked a reviewer for you, use r? to override)

@gnzlbg
Copy link
Contributor

gnzlbg commented Oct 10, 2019

cc @rkruppe

Is reading the MXCSR ok ? If not, why is that UB ?

@hanna-kruppe
Copy link

As far as LLVM is concerned:

The default LLVM floating-point environment assumes that floating-point instructions do not have side effects. Results assume the round-to-nearest rounding mode. No floating-point exception state is maintained in this environment.

This doesn't contain the magic words "behavior is undefined" but if you believe that writing MXCSR is UB, then this wording indicates that reading it is also UB because the UB flows from the assumption that the LLVM IR instructions are side-effect free and both reading and writing FP exception state conflicts with that.

@gnzlbg
Copy link
Contributor

gnzlbg commented Oct 26, 2019

This doesn't contain the magic words "behavior is undefined" but if you believe that writing MXCSR is UB, then this wording indicates that reading it is also UB because the UB flows from the assumption that the LLVM IR instructions are side-effect free and both reading and writing FP exception state conflicts with that.

What's the side-effect of reading the MXCSR register ?

@hanna-kruppe
Copy link

What's the side-effect of reading the MXCSR register ?

Reading doesn't have side effects AFAIK, but doing reads contradicts the assumption that floating point instructions don't have side effects (because they affect which value gets read from MXCSR).

@bors
Copy link
Contributor

bors commented Dec 14, 2021

☔ The latest upstream changes (presumably 0716b22) made this pull request unmergeable. Please resolve the merge conflicts.

@GabrielMajeri
Copy link
Contributor Author

I've rebased the PR, the build failures seem to be unrelated to the MXCSR changes.

@Amanieu
Copy link
Member

Amanieu commented Dec 19, 2021

The build failures should be fixed now, can you rebase again?

@GabrielMajeri
Copy link
Contributor Author

@Amanieu I've fixed some of the tests, but there are still others which use _MM_GET_EXCEPTION_STATE/_MM_SET_EXCEPTION_STATE (for example, test_mm_comieq_ss_vs_ucomieq_ss). Should I just get rid of those tests as well, or allow them to use the deprecated functions (even though it's sort-of undefined behavior)?

@Amanieu
Copy link
Member

Amanieu commented Dec 26, 2021

I think it's fine to leave the tests as they are for now and just allow the warning.

@Amanieu
Copy link
Member

Amanieu commented Dec 26, 2021

With that said, there have been changes in LLVM since this PR was opened in 2019 and I am not sure if the original reasons for deprecating these functions still apply.

@GabrielMajeri
Copy link
Contributor Author

@Amanieu Hmm, you're right, I'm not convinced using them is instantly undefined behavior anymore. I'm going to close this PR for now and leave a comment on the original issue, if someone's interested in further investigating the problem.

@GabrielMajeri GabrielMajeri deleted the deprecate-mxcsr branch January 2, 2022 14:02
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.

Deprecate MXCSR intrinsics
7 participants