#12915: cmmImplementSwitchPlans creates duplicate blocks -------------------------------------+------------------------------------- Reporter: alexbiehl | Owner: Type: bug | Status: new Priority: normal | Milestone: 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: | -------------------------------------+------------------------------------- Comment (by nomeata): `cmmImplementSwitchPlans` is run after `elimCommonBlocks` in order to avoid making unnecessary decisions in the branching code if the blocks happen to be the same ones any ways. I was not aware that `cmmImplementSwitchPlans` itself could make new blocks that are identical. In your examples, what happend to the assignments ` _sfh6::I64 = I64[_sfh3::P64 + 7];`? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12915#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#12915: cmmImplementSwitchPlans creates duplicate blocks -------------------------------------+------------------------------------- Reporter: alexbiehl | Owner: Type: bug | Status: new Priority: normal | Milestone: 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: | -------------------------------------+------------------------------------- Comment (by nomeata): Just guessing here, but we should probably not run sinking earlier, before `layoutStack`. We could run `elimCommonBlocks` again later, but that might be expensive and not very often kick in. Maybe with `-O2`? My gut feeling is that this comes up rather rarely. If someone wants to find evidence to the contrary: Add a late `elimCommonBlocks` pass, dump number of blocks before and after, run `nofib` and see how often it fires. Or (easier) report the change in code size. There is probably no obvious way of avoiding to generate these dead assignments in the first place? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12915#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#12915: cmmImplementSwitchPlans creates duplicate blocks -------------------------------------+------------------------------------- Reporter: alexbiehl | Owner: Type: bug | Status: new Priority: normal | Milestone: 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: | -------------------------------------+------------------------------------- Comment (by michalt): @nomeata: Yes, we definitely don't want to move the sinking pass before stack layout. Stack layout can generate a lot of unnecessary stuff that the sinking pass removes. There's also a `Node [Sinking after stack layout]` that explains why we don't currently run sinking twice (before and after stack layout). As for the comment in code: {{{ -- Any work storing block Labels must be performed _after_ -- elimCommonBlocks }}} IIUC it refers to things like proc point analysis that stores the labels for later use (and since `elimCommonBlock` can remove blocks/labels, it might no longer be safe to use those results). Anyway, I did a quick experiment and modified `CmmPipeline`: - Added a call to `elimCommonBlocks` directly after the sinking pass. - Duplicated the proc point analysis so that the actual splitting uses up to date information. Results: - Binary size: {{{ -1 s.d. ----- -1.0% +1 s.d. ----- -0.8% Average ----- -0.9% }}} - Module sizes: {{{ -1 s.d. ----- -1.8% +1 s.d. ----- +0.6% Average ----- -0.6% }}} - Compile times: {{{ -1 s.d. ----- -1.6% +1 s.d. ----- +3.3% Average ----- +0.8% }}} - Compiler allocations: {{{ -1 s.d. ----- +0.3% +1 s.d. ----- +1.0% Average ----- +0.7% }}} Runtime didn't seem to change (there were some outliers in the 1-2% area, but I think that's just noise). So it seems that there is actually some potential to decrease binary size, but I'm not sure it's worth the compile time overhead. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12915#comment:5 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#12915: cmmImplementSwitchPlans creates duplicate blocks -------------------------------------+------------------------------------- Reporter: alexbiehl | Owner: Type: bug | Status: new Priority: normal | Milestone: 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: | -------------------------------------+------------------------------------- Comment (by michalt): I'm also leaning towards closing. Trading ~1% compile time for a similar decrease of binary size doesn't sound like a great deal... @alexbiehl: Is this causing you some problems/performance issues? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12915#comment:7 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#12915: cmmImplementSwitchPlans creates duplicate blocks -------------------------------------+------------------------------------- Reporter: alexbiehl | Owner: Type: bug | Status: new Priority: normal | Milestone: 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: | -------------------------------------+------------------------------------- Comment (by rwbarton): I'm thinking the same as hsyl20. The binary size numbers indicate that whatever parts of the standard libraries end up getting linked into all the nofib programs got 1% smaller, which is nothing to sneeze at. Might also be worth checking out whether this is due to a few small gains and whether there might be a cheaper way of getting those gains. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12915#comment:9 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#12915: cmmImplementSwitchPlans creates duplicate blocks -------------------------------------+------------------------------------- Reporter: alexbiehl | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 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: | -------------------------------------+------------------------------------- Comment (by Ben Gamari ): In [changeset:"d5c4d46a62ce6a0cfa6440344f707136eff18119/ghc" d5c4d46a/ghc]: {{{ #!CommitTicketReference repository="ghc" revision="d5c4d46a62ce6a0cfa6440344f707136eff18119" CmmPipeline: add a second pass of CmmCommonBlockElim The sinking pass often gets rid of unnecessary registers registers/assignements exposing more opportunities for CBE, so this commit adds a second round of CBE after the sinking pass and should fix #12915 (and some examples in #14226). Nofib results: * Binary size: 0.9% reduction on average * Compile allocations: 0.7% increase on average * Runtime: noisy, two separate runs of nofib showed a tiny reduction on average, (~0.2-0.3%), but I think this is mostly noise * Compile time: very noisy, but generally within +/- 0.5% (one run faster, one slower) One interesting part of this change is that running CBE invalidates results of proc-point analysis. But instead of re-doing the whole analysis, we can use the map that CBE creates for replacing/comparing block labels (maps a redundant label to a useful one) to update the results of proc-point analysis. This lowers the overhead compared to the previous experiment in #12915. Signed-off-by: Michal Terepeta Test Plan: ./validate Reviewers: bgamari, simonmar Reviewed By: simonmar Subscribers: rwbarton, thomie, carter GHC Trac Issues: #12915, #14226 Differential Revision: https://phabricator.haskell.org/D4417 }}} -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12915#comment:11 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler