Fix atomicread/write operations
ClosedPublic

Authored by trommler on Sep 22 2017, 12:20 PM.

Details

Summary

In libraries/ghc-prim/cbits/atomic.c no barriers
were issued for atomic read and write operations. Starting with
gcc 4.7 compiler intrinsics are offered. The atomic intrinisics
are also available in clang. Use these to implement hs_atomicread*
and hs_atomicwrite.

Test Plan

validate on OSX and Windows

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
trommler created this revision.Sep 22 2017, 12:20 PM
dfeuer requested changes to this revision.Sep 25 2017, 12:05 AM
dfeuer added a subscriber: dfeuer.

Please add a note explaining what these are about. Do you think you can write a test that demonstrates the problem we had before?

This revision now requires changes to proceed.Sep 25 2017, 12:05 AM

Note: not having a test isn't fatal; it would be nice, but these things sometimes are too subtle to test easily enough. But comments are important.

simonmar edited edge metadata.Sep 26 2017, 1:54 PM

These get compiled into the same code on x86, right?

These get compiled into the same code on x86, right?

For atomic read the answer is yes.

For atomic write the same code is generated that is also generated by the native code generator.
Old:

.LFB2:
	.cfi_startproc
	movq	%rsi, (%rdi)
	ret

New:

.LFB3:
	.cfi_startproc
	movq	%rsi, (%rdi)
	mfence
	ret

From compiler/nativeGen/X86/CodeGen.hs:

genCCall _ _ (PrimTarget (MO_AtomicWrite width)) [] [addr, val] = do
    code <- assignMem_IntCode (intFormat width) addr val
    return $ code `snocOL` MFENCE
trommler updated this revision to Diff 14212.Sep 30 2017, 1:59 PM
  • add comment for memory barrier
trommler updated this revision to Diff 14213.Sep 30 2017, 2:03 PM
  • add lost commit
trommler edited the summary of this revision. (Show Details)Oct 2 2017, 10:24 AM
bgamari accepted this revision.Oct 25 2017, 1:30 PM

Alright, it's certainly no less correct than the previous state of things.

This revision was automatically updated to reflect the committed changes.