Use MO_Cmpxchg in Primops.cmm instead of ccall cas(..)
ClosedPublic

Authored by alexbiehl on Jul 27 2016, 11:39 AM.

Details

Summary

Adjust CmmParse.y to parse the cmpxchg{8, 16, 32, 64} instructions and use the 32 respectively the 64 bit variant in Primops.cmm. This effectively eliminates the compare-and-swap ccall to the rts.

Based off the mailing list question from @osa1 (https://mail.haskell.org/pipermail/ghc-devs/2016-July/012506.html).

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
cmm-inline-cas
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 10602
Build 12649: arc lint + arc unit
alexbiehl updated this revision to Diff 8338.Jul 27 2016, 11:39 AM
alexbiehl retitled this revision from to Use MO_Cmpxchg in Primops.cmm instead of ccall cas(..) This eliminates the ccall and directly inserts the Cmpxchg instruction..
alexbiehl updated this object.
alexbiehl edited the test plan for this revision. (Show Details)
alexbiehl retitled this revision from Use MO_Cmpxchg in Primops.cmm instead of ccall cas(..) This eliminates the ccall and directly inserts the Cmpxchg instruction. to Use MO_Cmpxchg in Primops.cmm instead of ccall cas(..).Jul 27 2016, 12:10 PM
alexbiehl updated this object.
alexbiehl edited edge metadata.
alexbiehl updated this object.
alexbiehl added a subscriber: osa1.
erikd edited edge metadata.EditedJul 27 2016, 3:48 PM

Code looks fine. Let me validate it on a PowerPC and Arm. Might take me a day or two.

simonmar requested changes to this revision.Jul 27 2016, 4:43 PM
simonmar edited edge metadata.

Great idea, I had been meaning to do this myself.

rts/PrimOps.cmm
43–48

Could put this in includes/Cmm.h to make it usable elsewhere.

44

I would rather have #define cmpxchgW cmpxhg32 so that we would write

(h) = prim %cmpxchgW(p,old,ew)

keeping it clear that it's still a primitive op, but just abstracting out the word size.

This revision now requires changes to proceed.Jul 27 2016, 4:43 PM
alexbiehl updated this revision to Diff 8341.Jul 27 2016, 11:44 PM
alexbiehl edited edge metadata.
alexbiehl updated this object.
  • Adjustments
erikd requested changes to this revision.Jul 28 2016, 3:12 AM
erikd edited edge metadata.

Builds fine on Armhf (which uses the LLVM backend) but fails on PowerPC when linking ghc-iserv with the following error:

/home/erikd/Git/ghc-upstream/rts/dist/build/libHSrts_thr.a(PrimOps.thr_o): In function `c14_info':
(.text+0x2b8): undefined reference to `hs_cmpxchg32'
/home/erikd/Git/ghc-upstream/rts/dist/build/libHSrts_thr.a(PrimOps.thr_o): In function `c5e_info':
(.text+0xac4): undefined reference to `hs_cmpxchg32'
/home/erikd/Git/ghc-upstream/rts/dist/build/libHSrts_thr.a(PrimOps.thr_o): In function `c8b_info':
(.text+0x1198): undefined reference to `hs_cmpxchg32'
/home/erikd/Git/ghc-upstream/rts/dist/build/libHSrts_thr.a(PrimOps.thr_o): In function `c8b_info':
(.text+0x122c): undefined reference to `hs_cmpxchg32'
/home/erikd/Git/ghc-upstream/rts/dist/build/libHSrts_thr.a(PrimOps.thr_o): In function `c8b_info':
(.text+0x12ec): undefined reference to `hs_cmpxchg32'

The command that produces that error is:

"inplace/bin/ghc-stage1" -o iserv/stage2/build/tmp/ghc-iserv -hisuf hi -osuf  o -hcsuf hc -static  -O0 -H64m -Wall -fllvm-fill-undef-with-garbage    -Werror    -hide-all-packages -i -iiserv/src -iiserv/stage2/build -Iiserv/stage2/build -iiserv/stage2/build/iserv/autogen -Iiserv/stage2/build/iserv/autogen     -optP-include -optPiserv/stage2/build/iserv/autogen/cabal_macros.h -package-id array-0.5.1.1 -package-id base-4.9.0.0 -package-id binary-0.8.3.0 -package-id bytestring-0.10.8.1 -package-id containers-0.5.7.1 -package-id deepseq-1.4.2.0 -package-id ghci-8.1 -package-id unix-2.7.2.0 -XHaskell2010  -threaded -no-hs-main -no-user-package-db -rtsopts      -Wnoncanonical-monad-instances  -odir iserv/stage2/build -hidir iserv/stage2/build -stubdir iserv/stage2/build    -optl-L'/home/erikd/Git/ghc-upstream/libraries/ghci/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/transformers/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/template-haskell/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/pretty/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/ghc-boot/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/ghc-boot-th/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/directory/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/unix/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/time/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/filepath/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/binar-package-db -rtsopts      -Wnoncanonical-monad-instances  -odir iserv/stage2/build -hidir iserv/stage2/build -stubdir iserv/stage2/build    -optl-L'/home/erikd/Git/ghc-upstream/libraries/ghci/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/transformers/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/template-haskell/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/pretty/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/ghc-boot/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/ghc-boot-th/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/directory/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/unix/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/time/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/filepath/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/binary/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/containers/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/bytestring/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/deepseq/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/array/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/base/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/integer-gmp/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/libraries/ghc-prim/dist-install/build' -optl-L'/home/erikd/Git/ghc-upstream/rts/dist/build' -optl-lrt -optl-lutil -optl-ldl -optl-lpthread -optl-lgmp -optl-lm -optl-lrt -optl-ldl -static  -O0 -H64m -Wall -fllvm-fill-undef-with-garbage    -Werror    -hide-all-packages -i -iiserv/src -iiserv/stage2/build -Iiserv/stage2/build -iiserv/stage2/build/iserv/autogen -Iiserv/stage2/build/iserv/autogen     -optP-include -optPiserv/stage2/build/iserv/autogen/cabal_macros.h -package-id array-0.5.1.1 -package-id base-4.9.0.0 -package-id binary-0.8.3.0 -package-id bytestring-0.10.8.1 -package-id containers-0.5.7.1 -package-id deepseq-1.4.2.0 -package-id ghci-8.1 -package-id unix-2.7.2.0 -XHaskell2010  -threaded -no-hs-main -no-user-package-db -rtsopts      -Wnoncanonical-monad-instances  iserv/stage2/build/Main.o iserv/stage2/build/GHCi/Utils.o iserv/stage2/build/cbits/iservmain.o

@simonmar Can you see anything odd there?

This revision now requires changes to proceed.Jul 28 2016, 3:12 AM
erikd added a subscriber: trommler.EditedJul 28 2016, 3:13 AM

@trommler Do you get the same result on PowerPC64?

includes/Cmm.h
186 ↗(On Diff #8341)

Do you really need the arguments here, or would #define cmpxchgW cmpxchg32 work?

alexbiehl marked 2 inline comments as done.Jul 28 2016, 5:32 AM
alexbiehl added inline comments.
includes/Cmm.h
186 ↗(On Diff #8341)

I thought it was a good documentation of the order you need to pass old and new. I can change it though, it still works without the arguments.

@simonmar I think we should do another commit with the changes to rts/package.conf.in which we merge in before this one.

This is the list of exports I would add:

hs_atomic_add8
hs_atomic_add16
hs_atomic_add32
hs_atomic_add64 # with if-def

hs_atomic_sub8
hs_atomic_sub16
hs_atomic_sub32
hs_atomic_sub64 # with if-def

hs_atomic_and8
hs_atomic_and16
hs_atomic_and32
hs_atomic_and64 # with if-def

hs_atomic_nand8
hs_atomic_nand16
hs_atomic_nand32
hs_atomic_nand64 # with if-def

hs_atomic_or8
hs_atomic_or16
hs_atomic_or32
hs_atomic_or64 # with if-def

hs_atomic_xor8
hs_atomic_xor16
hs_atomic_xor32
hs_atomic_xor64 # with if-def

hs_cmpxchg8
hs_cmpxchg16
hs_cmpxchg32
hs_cmpxchg64 # with if-def

hs_atomicread8
hs_atomicread16
hs_atomicread32
hs_atomicread64 # with if-def

hs_atomicwrite8
hs_atomicwrite16
hs_atomicwrite32
hs_atomicwrite64 # with if-def
alexbiehl updated this revision to Diff 8342.Jul 28 2016, 6:09 AM
alexbiehl edited edge metadata.
  • Remove arguments on cmpxchgW macro
In D2431#70796, @erikd wrote:

@trommler Do you get the same result on PowerPC64?

I see undefined symbols but in different places:

/home/peter/ghc/rts/dist/build/libHSrts_thr.a(PrimOps.thr_o): In function `.stg_casIntArrayzh':
(.text+0x3f0): undefined reference to `hs_cmpxchg64'
/home/peter/ghc/rts/dist/build/libHSrts_thr.a(PrimOps.thr_o): In function `.stg_casArrayzh':
(.text+0xe1c): undefined reference to `hs_cmpxchg64'
/home/peter/ghc/rts/dist/build/libHSrts_thr.a(PrimOps.thr_o): In function `.stg_casSmallArrayzh':
(.text+0x16e8): undefined reference to `hs_cmpxchg64'
/home/peter/ghc/rts/dist/build/libHSrts_thr.a(PrimOps.thr_o): In function `.stg_casMutVarzh':
(.text+0x17d0): undefined reference to `hs_cmpxchg64'
/home/peter/ghc/rts/dist/build/libHSrts_thr.a(PrimOps.thr_o): In function `c9U_entry':
(.text+0x18f0): undefined reference to `hs_cmpxchg64'

I see some updates to the diff since I applied it to my tree this morning (UTC+2). I'll try another build with the current diff.

erikd added a comment.Jul 28 2016, 3:06 PM

I don't think I've ever looked at the file "rts/package.conf.in" before.

Yes, adding , "-Wl,-u,hs_cmpxchg32" to the list and it built fine on PowerPC.

carter added a subscriber: carter.Jul 29 2016, 9:05 AM

Sweet! Does this mean uses of cas in the rts and user space won't have that extra function call indirection ?

Hrmm, these should be added to the Haskell layer primops too. With suitable state# token arguments similar to other pointer ish based read and write ops

@carter Yes, exactly that. Instead of a ccall to cas instructions like

0x0000000000027240 <+0>:     mov    %rsi,%rax
0x0000000000027243 <+3>:     lock cmpxchg %rdx,(%rdi)

are generated.

erikd added a comment.Jul 30 2016, 2:52 AM

@alexbiehl If you rebase this I'll test it on PowerPC.

alexbiehl updated this revision to Diff 8355.Jul 30 2016, 4:23 AM
alexbiehl edited edge metadata.

Rebase

erikd updated this object.Jul 30 2016, 4:41 AM
erikd edited edge metadata.

Builds on PowerPC 64-bit now.

erikd accepted this revision.Jul 31 2016, 3:35 AM
erikd edited edge metadata.

And on PowerPC.

bgamari accepted this revision.Jul 31 2016, 4:55 AM
bgamari edited edge metadata.

Thanks for all of the testing @erikd! And, of course, thanks for the implementation @alexbiehl.

erikd added a comment.Jul 31 2016, 5:08 AM

@bgamari No problem! I prefer it goes into master tested and working correctly rather than having to fix it after.

trommler accepted this revision.Aug 1 2016, 2:36 AM
trommler edited edge metadata.

Thank you Alex!

This revision was automatically updated to reflect the committed changes.
carter added a comment.Aug 3 2016, 7:42 PM

Is there a follow up patch or design plan for adding Haskell level siblings?

In D2431#71054, @carter wrote:

Is there a follow up patch or design plan for adding Haskell level siblings?

Not as far as I know but feel free to pick this up if @alexbiehl doesn't wish to do so himself.