Skip to content

replace mfence with lock'ed inst on x86#48123

Merged
vchuravy merged 2 commits intoJuliaLang:masterfrom
d-netto:dcn/mfence
Feb 13, 2023
Merged

replace mfence with lock'ed inst on x86#48123
vchuravy merged 2 commits intoJuliaLang:masterfrom
d-netto:dcn/mfence

Conversation

@d-netto
Copy link
Copy Markdown
Member

@d-netto d-netto commented Jan 4, 2023 •
edited
Loading

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
Copy Markdown
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

Keno
Keno approved these changes Jan 4, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Keno commented Jan 4, 2023

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

@vtjnash
Copy link
Copy Markdown
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
Copy Markdown
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.

vchuravy
vchuravy reviewed Jan 5, 2023
View reviewed changes
@brenhinkeller brenhinkeller added the compiler:codegen Generation of LLVM IR and native code label Jan 9, 2023
@vchuravy vchuravy mentioned this pull request Feb 10, 2023
Merged
@vchuravy vchuravy merged commit 7f4b78d into JuliaLang:master Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vchuravy vchuravy vchuravy left review comments

@Keno Keno Keno approved these changes

@vtjnash vtjnash Awaiting requested review from vtjnash

Assignees

No one assigned

Labels

compiler:codegen Generation of LLVM IR and native code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@d-netto @gbaraldi @Keno @vtjnash @vchuravy @brenhinkeller