Skip to content
Snippets Groups Projects

WorkWrap: Remove mkWWargs (#19874) and add regression test for #17819

Closed Sebastian Graf requested to merge wip/19874 into master

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).

Edited by Sebastian Graf

Merge request reports

Merge request pipeline #37492 passed with warnings

Merge request pipeline passed with warnings for 5b059106

Closed by Marge BotMarge Bot 3 years ago (Jun 27, 2021 6:57pm UTC)

Merge details

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Looks good. But a Lint error to investigate.

  • mentioned in issue #19874 (closed)

  • Sebastian Graf added 14 commits

    added 14 commits

    Compare with previous version

  • Sebastian Graf added 2 commits

    added 2 commits

    Compare with previous version

  • Sebastian Graf mentioned in merge request !5641 (closed)

    mentioned in merge request !5641 (closed)

  • Author Developer

    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.

    • Author Developer
      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.

  • Sebastian Graf added 2 commits

    added 2 commits

    Compare with previous version

  • Sebastian Graf changed the description

    changed the description

  • Author Developer

    Man, as always these "quick refactorings" turn out to be more tricky and time consuming than one would think.

  • Sebastian Graf added 2 commits

    added 2 commits

    Compare with previous version

  • Sebastian Graf added 2 commits

    added 2 commits

    Compare with previous version

  • Simon Peyton Jones
  • There is another problem, namely exactly the subject of Note [Join point and beta-redexes]:

    As you'll see above, I think we can avoid this problem, and the one about freshening, by deleting more code.

  • 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.

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading