Opened 9 years ago

Closed 4 years ago

Last modified 4 years ago

#2915 closed bug (fixed)

Arity is smaller than need be

Reported by: simonpj Owned by: simonpj
Priority: lowest Milestone: 7.6.2
Component: Compiler Version: 6.10.1
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Consider

h x = let f = case x of { True -> t1; False -> t2 }
          in (f,f)

where t1 and t2 have arity 1. You'd think that f should have arity 1, but it doesn't. (Reason: exprArity, used in Simplify.addNonRecWithUnf, doesn't look through the case.)

Fix this as part of the upcoming arity cleanup.

Simon

Change History (14)

comment:1 Changed 9 years ago by igloo

Milestone: 6.12 branch

comment:2 Changed 8 years ago by simonmar

Type of failure: Runtime performance bug

comment:3 Changed 7 years ago by igloo

Milestone: 6.12 branch6.12.3

comment:4 Changed 7 years ago by igloo

Milestone: 6.12.36.14.1
Priority: normallow

comment:5 Changed 7 years ago by igloo

Milestone: 7.0.17.0.2

comment:6 Changed 7 years ago by igloo

Milestone: 7.0.27.2.1

comment:7 Changed 6 years ago by igloo

Milestone: 7.2.17.4.1

comment:8 Changed 6 years ago by igloo

Milestone: 7.4.17.6.1
Priority: lowlowest

comment:9 Changed 5 years ago by igloo

Milestone: 7.6.17.6.2

comment:10 Changed 4 years ago by nomeata

Status: newinfoneeded

I don’t understand this ticket: Why should f have arity 1? It would be eta-expanded, and we’d be losing work!

comment:11 Changed 4 years ago by simonpj

If we eta-expand to:

h x = let f = \y -> case x of { True -> t1 y; False -> t2 y }
          in (f,f)

then we'd do the case x each time we call f, rather than once. But in exchange we get better code for f. Worth a try perhaps.

Notice that we do such things between lambdas:

h x = let g = \y -> case y of { True -> \v->e1; False -> \v->e2 } in ...

here we do (already) eta-expand g to

h x = let g = \y v -> case y of { True -> e1; False -> e2 } in ...

This seems inconsistent with not doing so in for f.

Simon

Last edited 4 years ago by simonpj (previous) (diff)

comment:12 Changed 4 years ago by nomeata

Reading more code I found this change (a522c3b25eea1fe40edae7052335acce75e8a1c3) introduced in response to #5625, which prevents this eta-expansion to be pedantic about bottoms.

The -fpedantic-bottoms check was added later. So I guess one could argue that if this flag is off, we should go back to the more liberal eta-expansion. I’ll try that and see what happens.

comment:13 Changed 4 years ago by nomeata

Resolution: fixed
Status: infoneededclosed

It seems that not test cases fail with this change, and only one nofib benchmark profits (mandel -1.9%). It would also simplify CoreArity (no need for the ae_bndrs field).

(Or course the scrunitee must not be expensive; I guard this using the ae_cheap_fn. If I don't do that, fluid improves allocation by -2.3%, but of course it's wrong. In most cases, though, FloatOut will have ensured that the scrutinee is cheap)

I’ll need to run another round of nofib, with an unloaded machine, to ensure that the runtimes are still ok... and here it is

            Min          -0.1%     -1.9%     -3.0%     -2.9%      0.0%
            Max          +0.0%     +0.0%     +3.5%     +2.2%     +3.8%
 Geometric Mean          -0.0%     -0.0%     -0.4%     -0.4%     +0.0%

Runtime varies a bit, with a slight tendency to an improvement. Guess that is good enough to push.

comment:14 Changed 4 years ago by Joachim Breitner <mail@…>

In 2931d19e90d2366f2ce308d65a36333336ca6059/ghc:

More liberally eta-expand a case-expression

at least with -fno-pedantic-bottoms. This fixes #2915, and undoes some
of a522c3b, on the grounds that with a flag `-fpedantic-bottoms`
around, we can be a bit more liberal when the flag is off..
Note: See TracTickets for help on using tickets.