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

Reverting PR 452 #509

Merged
merged 1 commit into from
Sep 25, 2023
Merged

Reverting PR 452 #509

merged 1 commit into from
Sep 25, 2023

Conversation

lemire
Copy link
Member

@lemire lemire commented Sep 25, 2023

PR #452 by @CharlesChen888 introduced an optimization that could be 'argued' to be safe. @feng-y reported problems and they blamed this PR.

I am not able to fault PR #452 when using mainstream compilers, but with the Intel compiler, we fail the tests (see my observation #506).

So I am reverting it.

It might be possible to make this safe under all compilers, but it will require more care.

@lemire lemire merged commit 7313c9d into master Sep 25, 2023
42 of 43 checks passed
@CharlesChen888
Copy link
Contributor

Is someone doing further inspections? I have little knowledge about Intel's compilers, and I am curious why PR #452 is causing problems.

@lemire
Copy link
Member Author

lemire commented Sep 26, 2023

@CharlesChen888 I am not sure it was ever safe. It may simply be a case where one compiler happens to expose a bug through our tests that is not triggered by other compilers.

Please see...
#476
where @feng-y report problems. They never provided a reproducible example, unfortunately.

If you take the latest version of the Intel compiler (ICX), and try to run our tests, you should be able to reproduce an issue. I am unable to demonstrate because the Intel compiler is not available in our CI, but I can assure you that the problem is real and reproducible. The inplace union gave the wrong answer, just like @feng-y reported.

If you could check it out with the Intel compiler (the supported on, ICX, not ICC), then that would be great. I am not going to work on this.

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