PeekTicket needs to be INLINE and shouldn't be #5

Open
rrnewton opened this Issue Jun 24, 2013 · 5 comments

Comments

Projects
None yet
2 participants
Owner

rrnewton commented Jun 24, 2013

Here is a standalone test

It can be reproduced with something like this:

GHC=ghc-7.6.3
cd ChaseLev
cabal-1.17.0_HEAD install --disable-executable-profiling --disable-library-profiling --disable-documentation --force-reinstalls ../AbstractDeque/ --with-ghc=$GHC
cabal-1.17.0_HEAD install --disable-executable-profiling --disable-library-profiling --disable-documentation --force-reinstalls ../AtomicPrimops/ --with-ghc=$GHC
$GHC -O2 --make RegressionTests/Issue5.hs -o Issue5.exe -main-is RegressionTests.Issue5.standalone_single_CAS
./Issue5.exe

The -O2 is required to make it fail (notice that --threaded is not). That's a big hint. (In fact, -O1 fails too.)

Owner

rrnewton commented Jun 25, 2013

This turned out to be a problem with allowing "peekTicket" to inline, which subsequently enables some surprising optimizations regarding unsafeCoerce. Perhaps what it comes down to is that unsafeCoerce needn't be respected in terms of a dataflow-dependencies, since it gets baked into the types.

This was closed as of 50a40fa

The solution was more NOINLINE annotations.

rrnewton closed this Jun 25, 2013

Owner

rrnewton commented Sep 2, 2015

This needs a core snippet to show what the problem was. In the meantime, here is some psuedocode off the top of my head:

case casMutVarTicketed# mv tick1 val1 s of 
 (# s2, success, tick2 #) -> 
   case casMutVarTicketed mv tick2 (1 + peekTicket tick2) of 
     ....

In this example, tick2 has type Any Int. That is what protects references to tick2 from having their pointer identity changed by GHC optimizations. If tick2 were a naked Int, GHC could unbox and rebox, resulting in a different pointer at tick2 argument to the second CAS.

The expression peekTicket tick2 would seem to have nothing to do with whether or not GHC can do these optimizations, right? Wrong. If the peekTicket function is not marked NOINLINE, then the type coercion gets inlined into the above expression, and GHC manages to learn that tick2 really is an Int, proceeding to unbox/rebox and break the program.

@tibbe -- is this what you would expect to happen?

Collaborator

tibbe commented Sep 2, 2015

Sounds sketchy but it might be a better question for SPJ.

Owner

rrnewton commented Sep 8, 2015

@mchakravarty confirmed that this is not a GHC bug, it's just how it is. I don't know the details, but something about there only being one entry in the type environment for tick2. Thus, if there is a visible coercion on tick2 (because peekTicket inlines), then the most specific type for tick2 will win, and GHC will learn tick2 :: Int, and do its pointer-changing unbox/rebox, which lead to this original issue.

This leaves me with the conclusion that our only good option is to update the CAS primop to return the same value twice with two different types (Any a and a). It wastes a register but avoids the out-of-line call to peekTicket and enables us to have two entries in that type environment Manuel mentioned. That is:

casMutVar#  :: MutVar# d a -> Any a -> a -> State# d -> (# State# d, Int#, Any a, a #)

Note that it still returns a boolean (Int#) indicating whether it succeeded or not, because it still internally needs to do a conditional to see if it needs to dirty something for the GHC. (@tibbe, you had some idea about avoiding this to get closer to exposing the raw, single instruction?)

CC'ing @ekmett to get his opinion since he's a heavy user of CAS in GHC, and @fryguybob who may have input on this design.

rrnewton reopened this Sep 8, 2015

rrnewton changed the title from Consistent failure of a push/pop when debug wrapper is on to PeekTicket needs to be INLINE and shouldn't be Sep 8, 2015

Owner

rrnewton commented Sep 8, 2015

Reopening until this is fixed "properly".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment