Make lambda fit for MFP
ClosedPublic

Authored by sgraf on Aug 10 2018, 11:17 AM.

Details

Reviewers
RyanGlScott
bgamari
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rNOFIB7cbfbbed8b6d: Make lambda fit for MFP
Summary

The next step of the MonadFail Proposal broke nofib's lambda benchmark.
This commit fixes that in an unintrusive way.

Diff Detail

Repository
rNOFIB nofib
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sgraf created this revision.Aug 10 2018, 11:17 AM
Owners added a reviewer: Restricted Owners Package.Aug 10 2018, 11:17 AM
RyanGlScott added inline comments.Aug 10 2018, 11:38 AM
spectral/lambda/Main.hs
112

Let's not do this. I would prefer replacing these uses of fail with error directly, instead of relying on overlapping instances (which is questionable at best).

174

Similarly with Id.

sgraf added inline comments.Aug 10 2018, 11:42 AM
spectral/lambda/Main.hs
112

I tried that, but this still doesn't get rid of the two non-exhaustive patterns in binds here. I could rewrite them, I guess. It's up to you.

RyanGlScott added inline comments.Aug 10 2018, 11:54 AM
spectral/lambda/Main.hs
112

I'm confused. That function is polymorphic over EvalEnvMonad, so shouldn't the MonadFail superclass for EvalEnvMonad take care of those?

RyanGlScott added inline comments.Aug 10 2018, 11:59 AM
spectral/lambda/Main.hs
112

Ah, now I see what you mean. I didn't realize that State Env and Id were also instances of EvalEnvMonad, so adding a MonadFail superclass would force them to have MonadFail instances as well. Ugh.

Instead of doing that, perhaps a more straightforward fix would be to replace those two failable traverseTerm lines with an auxiliary function like:

traverseCon u = do
  u' <- traverseTerm u
  case u' of
    Con c -> return c
    _ -> error $ "traverseCon: " ++ show u'
sgraf updated this revision to Diff 17650.Aug 12 2018, 1:19 PM

Fix it without overlapping instances

RyanGlScott accepted this revision.Aug 13 2018, 9:09 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Aug 13 2018, 9:09 AM

This looks like a step forward, but I'm getting

Main.hs:183:9: error:
    • No instance for (Control.Monad.Fail.MonadFail Id)
        arising from a do statement
        with the failable pattern ‘Con v'’
    • In a stmt of a 'do' block: Con v' <- simpleEval env v
      In the expression:
        do Con u' <- simpleEval env u
           Con v' <- simpleEval env v
           return (Con (u' + v'))
RyanGlScott requested changes to this revision.Aug 17 2018, 9:21 PM

Indeed, it looks like we need a simpleEvalCon counterpart to traverseCon as well.

This revision now requires changes to proceed.Aug 17 2018, 9:21 PM

Are you planning to finish this, @sgraf?

Sorry, I was on vacation. I'll finish this in a moment.

sgraf updated this revision to Diff 17747.Aug 22 2018, 11:25 AM

Add simpleEvalCon

This revision is now accepted and ready to land.Aug 22 2018, 12:12 PM
This revision was automatically updated to reflect the committed changes.

Someone needs to update the submodule in ghc to unblock https://perf.haskell.org/ghc/

@nomeata I've updated nofib submodule in b1f5d2fdbf.