Skip to content

Use new 64-bit interlocked intrinsics on x86 #986

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.

Sign up for GitHub

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

Jump to bottom
Merged
CaseyCarter merged 7 commits into from
Jul 7, 2020
Merged

Use new 64-bit interlocked intrinsics on x86 #986

CaseyCarter merged 7 commits into from
Jul 7, 2020

Conversation

@AlexGuteniev
Copy link
Contributor

@AlexGuteniev AlexGuteniev commented Jul 4, 2020 •
edited
Loading

Resolves #965

Looks like red only diff change.
Such changes are more likely to be helpful than green diff changes. unfortunately, did not succeed in it, clang does not have such intrinsics.

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner July 4, 2020 08:57
@StephanTLavavej StephanTLavavej added the performance Must go faster label Jul 4, 2020
@AlexGuteniev
Copy link
Contributor Author

AlexGuteniev commented Jul 4, 2020

Wow, some tests do fail. Will take a look

@AlexGuteniev AlexGuteniev mentioned this pull request Jul 4, 2020
Closed
@AlexGuteniev
Copy link
Contributor Author

AlexGuteniev commented Jul 4, 2020

So actually clang does not support them.
It was an error in experiment, I actually tried cl instead of clang-cl/

StephanTLavavej
StephanTLavavej requested changes Jul 4, 2020
View reviewed changes
AlexGuteniev and others added 2 commits July 5, 2020 05:25
@AlexGuteniev @StephanTLavavej
Update stl/inc/atomic
9becdbe 
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
@AlexGuteniev
proper comments style
80576ba
@AlexGuteniev
Copy link
Contributor Author

AlexGuteniev commented Jul 6, 2020

Filled LLVM-46595.

StephanTLavavej
StephanTLavavej approved these changes Jul 6, 2020
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Thanks for filing the LLVM bug/feature request!

We haven't repeated bug numbers in #else arrow comments before, but (1) I think this variation is acceptable (I'm trying to be less draconian about consistency when it isn't critical 😸), (2) it makes it super clear which side is the workaround which is nice, and (3) we have an active issue about preprocessor comment conventions anyways.

Let's ship this. 🚀

@BillyONeal BillyONeal self-assigned this Jul 7, 2020
@CaseyCarter CaseyCarter assigned CaseyCarter and unassigned BillyONeal Jul 7, 2020
@CaseyCarter CaseyCarter merged commit 7253921 into microsoft:master Jul 7, 2020
@CaseyCarter
Copy link
Contributor

CaseyCarter commented Jul 7, 2020

Thanks for making 64-bit atomics more efficient on x86!

@CaseyCarter CaseyCarter removed their assignment Jul 7, 2020
@AlexGuteniev AlexGuteniev deleted the atomic_64_common_intrinsics branch July 8, 2020 02:13
@AlexGuteniev
Copy link
Contributor Author

AlexGuteniev commented Jul 8, 2020

LLVM-46595 is fixed surprisingly quickly. Here's a commit: llvm/llvm-project@82206e7

So, the next step is probably waiting for the fixed version of LLVM to get into Visual Studio distribution?

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Jul 8, 2020

LLVM-46595 is fixed surprisingly quickly. Here's a commit: llvm/llvm-project@82206e7

So, the next step is probably waiting for the fixed version of LLVM to get into Visual Studio distribution?

Yes. I've filed #1018 to track removal of workarounds.

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Aug 22, 2020
@StephanTLavavej
Revert 64-bit interlocked intrinsics on x86
fe3267a 
This ports MSVC-PR-269581, reverting microsoftGH-986 / MSVC-PR-240462.

According to John Morgan:

> We recently discovered a conflict between the 64-bit Interlocked
> intrinsic functions newly implemented for x86 and the Windows 8.1 SDK.
> To resolve the conflict we are reverting the change. We may later
> implement an alternative solution to support these functions on x86.
@StephanTLavavej StephanTLavavej mentioned this pull request Aug 22, 2020
Merged
StephanTLavavej added a commit that referenced this pull request Aug 26, 2020
@StephanTLavavej
Revert 64-bit interlocked intrinsics on x86 (#1222)
2bec360 
This ports MSVC-PR-269581, reverting GH-986 / MSVC-PR-240462.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+2 more reviewers

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@BillyONeal BillyONeal BillyONeal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

performance Must go faster

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<atomic>: Use new 64-bit interlocked intrinsics on x86

4 participants

@AlexGuteniev @CaseyCarter @BillyONeal @StephanTLavavej