Opened 9 months ago

Closed 8 months ago

Last modified 8 months ago

#8091 closed bug (fixed)

retry# lacks strictness information

Reported by: parcs Owned by:
Priority: normal Milestone:
Component: Compiler Version: 7.6.3
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty: Unknown
Test Case: Blocked By:
Blocking: Related Tickets:

Description

The retry# primop should be marked as returning bottom since it never actually returns anything as far as the simplifier is concerned. This change would help the simplifier eliminate unreachable code inside STM transactions.

Change History (6)

comment:1 Changed 9 months ago by parcs

  • Status changed from new to patch

The patch (validated):

  • compiler/prelude/primops.txt.pp

    diff --git a/compiler/prelude/primops.txt.pp b/compiler/prelude/primops.txt.pp
    index 3d3518b..e12c1a5 100644
    a b primop AtomicallyOp "atomically#" GenPrimOp 
    16141614primop  RetryOp "retry#" GenPrimOp 
    16151615   State# RealWorld -> (# State# RealWorld, a #) 
    16161616   with 
     1617   strictness  = { \ _arity -> mkStrictSig (mkTopDmdType [topDmd] botRes) } 
    16171618   out_of_line = True 
    16181619   has_side_effects = True 
    16191620 

The unfoldings in Control.Concurrent.STM.TSem and .TQueue are improved as a result of this change due to the aforementioned dead code elimination.

comment:2 follow-up: Changed 9 months ago by simonpj

I like the idea, but it does have strictness consequences too. For example:

f :: TVar Int -> Int -> STM Int
f r y = do { v <- readTVar r
           ; if v>0 then retry
                    else if y>0 then return 0 else return y
           }

I have not checked but I think this will not be strict in y because the then branch doesn't evaluate y. With your change it'll become strict in y, just as it would if you replace retry with throw exn. I think that's probably fine, and no one will even notice; just saying.

Simon

comment:3 in reply to: ↑ 2 Changed 9 months ago by parcs

Ah, subtle..

But in this particular example , f seems to be lazy in y even when retry is replaced with throw Overflow: its strictness signature is <S,1*U(U)><L,1*U(U)><L,U> either way..

(It would be incorrect, I think, to have f strict in y since that could potentially elide the side-effect-having call to readTVar.)

I found an example of a function whose strictness does change due to this patch (courtesy of the comments for RaiseIOOp in primops.txt.pp):

g :: Int -> Int -> STM Int
g x y | x>0       = retry
      | y>0       = return 1
      | otherwise = return 2

This function is now strict in y, where it originally wasn't. It looks like this change will only have strictness implications in fairly contrived functions where retry is used in such a way that the transaction could not possibly make any progress; and in that case behaving as if retry returns bottom makes a good amount of sense.

Version 0, edited 9 months ago by parcs (next)

comment:4 Changed 9 months ago by parcs

To be clear, the form of dead code elimination that this change enables is

replacing

case retry# s1 of
    (# s2, a #) -> ...inaccessible..

with simply

retry# s1

Such Core can appear after performing case-of-case on code like

s <- readTVar v
when (s > 0) retry
...

where everything after the when statement might get inlined under the scrutinization of retry.

comment:5 Changed 8 months ago by thoughtpolice

  • Resolution set to fixed
  • Status changed from patch to closed

Thanks Patrick. Committed in

commit e2b72cadce3ae799a74981f9957b40bb201f1ba1
Author: Austin Seipp <aseipp@pobox.com>
Date:   Sat Aug 10 22:39:15 2013 -0500

    Mark retry# as returning bottom.
    
    This change helps the simplifier eliminate unreachable code, since
    retry# technically doesn't return.
    
    This closes ticket #8091.
    
    Authored-by: Patrick Palka <patrick@parcs.ath.cx>
    Signed-off-by: Austin Seipp <aseipp@pobox.com>

comment:6 Changed 8 months ago by simonpj

I wonder if we might have a regression test here... Perhaps a -ddump-simpl and check that a function in the ...inaccessible... part no longer appears?

Simon

Note: See TracTickets for help on using tickets.