[GHC] #13160: Simplify CoreFV.FVAnn
#13160: Simplify CoreFV.FVAnn -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.0.1 Keywords: | Operating System: Unknown/Multiple Architecture: | Type of failure: None/Unknown Unknown/Multiple | Test Case: | Blocked By: Blocking: | Related Tickets: Differential Rev(s): | Wiki Page: -------------------------------------+------------------------------------- When introducing type-in-type, Richard elaborated the free varaible finder so that it finds three things at once (in `CoreFV`): {{{ data FVAnn = FVAnn { fva_fvs :: DVarSet -- free in expression , fva_ty_fvs :: DVarSet -- free only in expression's type , fva_ty :: Type -- expression's type } }}} I think `fva_ty` is needed only to support `fva_ty_fvs`. And `fva_ty_fvs` seems to be used only to support the calls to `freeVarsOfType` in `FloatIn`. And those calls in turn are only to suppor `used_in_ty` in `FloatIn.sepBindsByDropPoint`. In conversation yesterday, Richard and I agreed that all this is unnecessary. Coercion bindings simply do not float inwards, so we do not need to take these precautions. (We should add a Note to explain why they don't float, and what problem might arise if they did.) To confirm this I set `used_in_ty` to `False` and compiled from scratch; everything is fine. So I propose that we * get rid of the `ty_fvs` argument to `sepBindsByDropPoint` * simplfy `FVAnn` to just gather free variables (ie one field only) Result: it's all simpler. Richard will do this when he gets a moment. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/13160> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#13160: Simplify CoreFV.FVAnn -------------------------------------+------------------------------------- Reporter: simonpj | Owner: goldfire Type: task | Status: new Priority: high | Milestone: 8.2.1 Component: Compiler | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by simonpj): * priority: normal => high * owner: => goldfire * type: bug => task * milestone: => 8.2.1 @@ -30,1 +30,1 @@ - Richard will do this when he gets a moment. + Richard will do this when he gets a moment. But I really hope for 8.2 New description: When introducing type-in-type, Richard elaborated the free varaible finder so that it finds three things at once (in `CoreFV`): {{{ data FVAnn = FVAnn { fva_fvs :: DVarSet -- free in expression , fva_ty_fvs :: DVarSet -- free only in expression's type , fva_ty :: Type -- expression's type } }}} I think `fva_ty` is needed only to support `fva_ty_fvs`. And `fva_ty_fvs` seems to be used only to support the calls to `freeVarsOfType` in `FloatIn`. And those calls in turn are only to suppor `used_in_ty` in `FloatIn.sepBindsByDropPoint`. In conversation yesterday, Richard and I agreed that all this is unnecessary. Coercion bindings simply do not float inwards, so we do not need to take these precautions. (We should add a Note to explain why they don't float, and what problem might arise if they did.) To confirm this I set `used_in_ty` to `False` and compiled from scratch; everything is fine. So I propose that we * get rid of the `ty_fvs` argument to `sepBindsByDropPoint` * simplfy `FVAnn` to just gather free variables (ie one field only) Result: it's all simpler. Richard will do this when he gets a moment. But I really hope for 8.2 -- -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/13160#comment:1> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#13160: Simplify CoreFV.FVAnn -------------------------------------+------------------------------------- Reporter: simonpj | Owner: bgamari Type: task | Status: new Priority: high | Milestone: 8.2.1 Component: Compiler | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3170 Wiki Page: | -------------------------------------+------------------------------------- Changes (by bgamari): * owner: goldfire => bgamari * differential: => Phab:D3170 Comment: I tried to knock this one off while waiting for a build a few days ago, but it's not clear that the patch is quite correct and certainly lacks documentation. I'll need to come back to this. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/13160#comment:2> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#13160: Simplify CoreFV.FVAnn -------------------------------------+------------------------------------- Reporter: simonpj | Owner: bgamari Type: task | Status: new Priority: high | Milestone: 8.4.1 Component: Compiler | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3170 Wiki Page: | -------------------------------------+------------------------------------- Changes (by bgamari): * milestone: 8.2.1 => 8.4.1 Comment: This won't happen for 8.2. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/13160#comment:3> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#13160: Simplify CoreFV.FVAnn -------------------------------------+------------------------------------- Reporter: simonpj | Owner: bgamari Type: task | Status: new Priority: high | Milestone: 8.4.1 Component: Compiler | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3170 Wiki Page: | -------------------------------------+------------------------------------- Description changed by mpickering: Old description:
When introducing type-in-type, Richard elaborated the free varaible finder so that it finds three things at once (in `CoreFV`): {{{ data FVAnn = FVAnn { fva_fvs :: DVarSet -- free in expression , fva_ty_fvs :: DVarSet -- free only in expression's type , fva_ty :: Type -- expression's type } }}} I think `fva_ty` is needed only to support `fva_ty_fvs`. And `fva_ty_fvs` seems to be used only to support the calls to `freeVarsOfType` in `FloatIn`. And those calls in turn are only to suppor `used_in_ty` in `FloatIn.sepBindsByDropPoint`.
In conversation yesterday, Richard and I agreed that all this is unnecessary. Coercion bindings simply do not float inwards, so we do not need to take these precautions. (We should add a Note to explain why they don't float, and what problem might arise if they did.)
To confirm this I set `used_in_ty` to `False` and compiled from scratch; everything is fine.
So I propose that we
* get rid of the `ty_fvs` argument to `sepBindsByDropPoint` * simplfy `FVAnn` to just gather free variables (ie one field only)
Result: it's all simpler.
Richard will do this when he gets a moment. But I really hope for 8.2
New description: When introducing type-in-type, Richard elaborated the free variable finder so that it finds three things at once (in `CoreFV`): {{{ data FVAnn = FVAnn { fva_fvs :: DVarSet -- free in expression , fva_ty_fvs :: DVarSet -- free only in expression's type , fva_ty :: Type -- expression's type } }}} I think `fva_ty` is needed only to support `fva_ty_fvs`. And `fva_ty_fvs` seems to be used only to support the calls to `freeVarsOfType` in `FloatIn`. And those calls in turn are only to suppor `used_in_ty` in `FloatIn.sepBindsByDropPoint`. In conversation yesterday, Richard and I agreed that all this is unnecessary. Coercion bindings simply do not float inwards, so we do not need to take these precautions. (We should add a Note to explain why they don't float, and what problem might arise if they did.) To confirm this I set `used_in_ty` to `False` and compiled from scratch; everything is fine. So I propose that we * get rid of the `ty_fvs` argument to `sepBindsByDropPoint` * simplfy `FVAnn` to just gather free variables (ie one field only) Result: it's all simpler. Richard will do this when he gets a moment. But I really hope for 8.2 -- -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/13160#comment:4> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#13160: Simplify CoreFV.FVAnn -------------------------------------+------------------------------------- Reporter: simonpj | Owner: bgamari Type: task | Status: new Priority: high | Milestone: 8.4.1 Component: Compiler | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3170 Wiki Page: | -------------------------------------+------------------------------------- Comment (by Simon Peyton Jones <simonpj@…>): In [changeset:"751996e90a964026a3f86853338f10c82db6b610/ghc" 751996e/ghc]: {{{ #!CommitTicketReference repository="ghc" revision="751996e90a964026a3f86853338f10c82db6b610" Kill off complications in CoreFVs When doing type-in-type, Richard introduce some substantial complications in CoreFVs, gathering types and free variables of type. In Trac #13160 we decided that this complication was unnecessary, so this patch removes it. Unfortnately I then fell down a twisty rabbit hole. Roughly: * An apparently-innocuous change in the AnnApp case of fiExpr made the fuction a little bit stricter, so we ended up peering into the arguments when we didn't before (namely when there are no bindings being floated inwards). I've rejigged it so that it's not fragile any more. * Peering into the arguments was sometimes expensive, becuase exprIsExpandable can be expensive because it looks deeply into the expression. * The combination of the two led to a non-linear explosion of work when the argument of a function is a deeep nest of constructors. This bug has been lurking for ages. I solved it by replacing exprIsExpandable with exprIsHNF + exprIsTrivial; see Note [noFloatInto considerations] * The code around floating case-expressions turned out to be very delicate, because can_fail primops (which we want to float inwards) can't be floated outwards; so we have to be careful never to float them too far. Note [Floating primops] has the details * I ended up refactoring some rather opaque code in sepBindsByDropPoint. Working all this out meant that I rewrote quite a bit of code, so it's now a reasonably substantial patch. But it's a net improvement. }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/13160#comment:5> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#13160: Simplify CoreFV.FVAnn -------------------------------------+------------------------------------- Reporter: simonpj | Owner: bgamari Type: task | Status: closed Priority: high | Milestone: 8.4.1 Component: Compiler | Version: 8.0.1 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3170 Wiki Page: | -------------------------------------+------------------------------------- Changes (by simonpj): * status: new => closed * resolution: => fixed -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/13160#comment:6> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
participants (1)
-
GHC