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

replace mfence with lock'ed inst on x86 #48123

Merged
merged 2 commits into from
Feb 13, 2023
Merged

Conversation

d-netto
Copy link
Member

@d-netto d-netto commented Jan 4, 2023

A dummy instruction with lock prefix should provide the same sequential consistency guarantees as an mfence on x86.

This had a large performance impact when benchmarking work-stealing queues for parallel marking and it would be interesting to see how/if it affects performance in general.

CC: @vchuravy

@d-netto d-netto requested a review from vtjnash January 4, 2023 19:52
@gbaraldi
Copy link
Member

gbaraldi commented Jan 4, 2023

This seems to be fine, linux changed to this a little while ago https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=450cbdd0125cfa5d7bbf9e2a6b6961cc48d29730
We might want to do the exact same instruction they do since apparently they did some effort on what's better

Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

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

Though as the kernel people, I'm not sure why the compiler doesn't just do this and I agree with @gbaraldi that it makes sense to align with the Linux-kernel instructions.

@Keno
Copy link
Member

Keno commented Jan 4, 2023

Looks like LLVM has support to emit this since https://reviews.llvm.org/D58632 and even emits it in the fallback case when the processor has no mfence. I don't know why it still emits mfence for the llvm fence intrinsic other than that nobody bothered updating it.

@Keno
Copy link
Member

Keno commented Jan 4, 2023

@d-netto want to submit an upstream PR to remove the mfence case to see what they think?

@d-netto
Copy link
Member Author

d-netto commented Jan 4, 2023

I believe @vchuravy already submitted a patch to LLVM to replace thefence intrinsic with the locked instruction.

@Keno
Copy link
Member

Keno commented Jan 4, 2023

Ah indeed: https://reviews.llvm.org/D129947

@vtjnash
Copy link
Member

vtjnash commented Jan 5, 2023

I may possibly say we should rather just wait for that to catch up with gcc and https://reviews.llvm.org/D129947, than worry about maintaining this if something changes again

@vchuravy
Copy link
Member

vchuravy commented Jan 5, 2023

It will take a long time for the compilers to catch up. It landed in GCC12 and it will be a second before we get it into Clang.

src/julia_atomics.h Outdated Show resolved Hide resolved
@brenhinkeller brenhinkeller added the compiler:codegen Generation of LLVM IR and native code label Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants