mkWWargs
's job was pushing casts inwards and doing eta expansion to match
the arity with the number of argument demands we w/w for.
Nowadays, we use the Simplifier to eta expand to arity. In fact, in recent years we have even seen the eta expansion done by w/w as harmful, see Note [Don't eta expand in w/w]. If a function hasn't enough manifest lambdas, don't w/w it!
What purpose does mkWWargs
serve in this world? Not a great one, it turns out!
I could remove it by pulling some important bits,
notably Note [Freshen WW arguments] and Note [Join points and beta-redexes].
Result: We reuse the freshened binder names of the wrapper in the
worker where possible (see testuite changes), much nicer!
In order to avoid scoping errors due to lambda-bound unfoldings in worker arguments, we zap those unfoldings now. In doing so, we fix #19766 (closed).
Fixes #19874 (closed).
Merge request reports
Activity
added worker/wrapper transformation label
- Resolved by Sebastian Graf
mentioned in issue #19874 (closed)
added 14 commits
-
e7e8384e...ef4d2999 - 11 commits from branch
master
- 3ed1b9c5 - Enhance cast worker/wrapper for INLINABLE
- e6d53186 - WorkWrap: Remove mkWWargs (#19874 (closed))
- 4ac3874e - Add regression test for #17819 (closed)
Toggle commit list-
e7e8384e...ef4d2999 - 11 commits from branch
added 2 commits
- 62af8ed8 - WorkWrap: Remove mkWWargs (#19874 (closed))
- f35184db - Add regression test for #17819 (closed)
mentioned in merge request !5641 (closed)
The first commit of the batch is !5820 (closed) at the moment. I'll rebase once that one lands.
- Resolved by Sebastian Graf
Result: We reuse the binder names of the wrapper in the worker where possible (see testuite changes), much nicer!
I don't have all the details paged in but there is the
Note [Freshen WW arguments]
which says:Wen we do a worker/wrapper split, we must not in-scope names as the arguments of the worker, else we'll get name capture.
Perhaps it is ok to re-use the wrapper names, but they will most definitely always be in scope. I don't think that's the case the note is worrying about. But the note is a bit vague on that and right now it's description at a glance seems at odds with the summary of this patch.
Maybe you could clarify that if you already looked into that.
- Resolved by Sebastian Graf
There is another problem, namely exactly the subject of
Note [Join point and beta-redexes]
:Originally, the worker would invoke the original function by calling it with arguments, thus producing a beta-redex for the simplifier to munch away: \x y z -> e => (\x y z -> e) wx wy wz Now that we have special rules about join points, however, this is Not Good if the original function is itself a join point, as then it may contain invocations of other join points: join j1 x = ... join j2 y = if y == 0 then 0 else j1 y => join j1 x = ... join $wj2 y# = let wy = I# y# in (\y -> if y == 0 then 0 else jump j1 y) wy join j2 y = case y of I# y# -> jump $wj2 y# There can't be an intervening lambda between a join point's declaration and its occurrences, so $wj2 here is wrong. But of course, this is easy enough to fix: ... let join $wj2 y# = let wy = I# y# in let y = wy in if y == 0 then 0 else j1 y ... Hence we simply do the beta-reduction here. (This would be harder if we had to worry about hygiene, but luckily wy is freshly generated.)
So it seems we can't simply generate an application like
(\x y -> e) x y
. Yet, the patch doesn't currently trigger any CoreLint errors.It seems that
mkWWargs
did some pretty simple beta reduction on the fun body expression on the fly... Maybe we should do the same. I think it would also solve some invalidated occurrence info hiccups.
added 2 commits
- 113564af - WorkWrap: Remove mkWWargs (#19874 (closed))
- 6a291c9a - Add regression test for #17819 (closed)
added 2 commits
- 207cb6fd - WorkWrap: Remove mkWWargs (#19874 (closed))
- 6e460bb6 - Add regression test for #17819 (closed)
added 2 commits
- 1a02f25f - WorkWrap: Remove mkWWargs (#19874 (closed))
- 360a5356 - Add regression test for #17819 (closed)
- Resolved by Sebastian Graf
- Resolved by Sebastian Graf
But we apply the same worker body context to the stable unfolding and we can't assume that the stable unfolding calls it arguments x and y, too. That's what I'm trying to say in the last paragraph of the haddock.
A very good point, which I totally didn't get.
You propose a decent solution too.