Support SCC pragmas in declaration context
ClosedPublic

Authored by osa1 on Jul 18 2016, 6:11 AM.

Details

Summary

Not having SCCs at the top level is becoming annoying real quick. For simplest
cases, it's possible to do this transformation:

f x y = ...
=>
f = {-# SCC f #-} \x y -> ...

However, it doesn't work when there's a where clause:

f x y = <t is in scope>
  where t = ...
=>
f = {-# SCC f #-} \x y -> <t is out of scope>
  where t = ...

Or when we have a "equation style" definition:

f (C1 ...) = ...
f (C2 ...) = ...
f (C3 ...) = ...
...

(usual solution is to rename f to f' and define a new f with a SCC)

This patch implements support for SCC annotations in declaration contexts. This
is now a valid program:

f x y = ...
  where
    g z = ...
    {-# SCC g #-}
{-# SCC f #-}
Test Plan

This passes slow validate (no new failures added).

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
toplevel_scc
Lint
Lint SkippedExcuse: later
Unit
No Unit Test Coverage
Build Status
Buildable 10496
Build 12543: arc lint + arc unit
osa1 updated this revision to Diff 8232.Jul 18 2016, 6:11 AM
osa1 retitled this revision from to Implement SCC_FUNCTION pragma.
osa1 updated this object.
osa1 edited the test plan for this revision. (Show Details)
bgamari requested changes to this revision.Jul 18 2016, 6:23 AM
bgamari edited edge metadata.

Is this ready for review? Did you explore the possibility of overloading the existing SCC pragma instead of introducing SCC_FUNCTION? The latter seems rather long and really only benefits the parser.

How about some documentation in the users guide with an example?

This revision now requires changes to proceed.Jul 18 2016, 6:23 AM
osa1 added a comment.Jul 18 2016, 6:38 AM

@bgamari this is just a prototype, I'll send an email to ghc-devs for the discussion.

osa1 updated this revision to Diff 8246.Jul 18 2016, 4:22 PM
osa1 edited edge metadata.
  • rebase
osa1 updated this revision to Diff 8249.Jul 19 2016, 3:26 AM
osa1 edited edge metadata.
  • parser improvements
osa1 updated this revision to Diff 8250.Jul 19 2016, 3:45 AM
osa1 edited edge metadata.
  • test
osa1 updated this revision to Diff 8251.Jul 19 2016, 3:54 AM
osa1 edited edge metadata.
  • revert
  • tests
osa1 updated this revision to Diff 8253.Jul 19 2016, 4:01 AM
osa1 edited edge metadata.

rebase

simonmar requested changes to this revision.Jul 19 2016, 5:09 AM
simonmar added a reviewer: simonmar.
simonmar added a subscriber: simonmar.

I like this! Just a couple of inline comments

testsuite/driver/testlib.py
1199 ↗(On Diff #8253)

debugging?

1244 ↗(On Diff #8253)

This doesn't look right to me, extra_run_opts is for args passed to the program, not to the RTS.

This revision now requires changes to proceed.Jul 19 2016, 5:09 AM

Oh, and you need to add it to the User Guide too.

osa1 added inline comments.Jul 19 2016, 5:19 AM
testsuite/driver/testlib.py
1199 ↗(On Diff #8253)

Yes, sorry this patch is not quite ready for reviews... See the other comment.

1244 ↗(On Diff #8253)

Right. Basically I need testsuite to look for .sample file in my new test, but since it doesn't run in prof way (because I don't want to pass -fprof-auto) it just ignores my .sample file. Ping @thomie .

osa1 updated this revision to Diff 8256.Jul 19 2016, 6:19 AM
osa1 edited edge metadata.
  • revert the testsuite breakage. minor refactoring.
thomie added inline comments.Jul 19 2016, 8:40 AM
testsuite/config/ghc
144 ↗(On Diff #8256)

This shouldn't be necessary. By default the testsuite driver runs only the profasm and profthreaded test ways (see earlier in this file). Other (prof) ways you have to ask for explicitly, using extra_ways as you do below.

The prof_ways variable is sometimes used to omit all profiling ways: omit_ways(prof_ways).

testsuite/tests/profiling/should_run/all.T
119 ↗(On Diff #8256)

-prof isn't necessary here.

Yes, sorry this patch is not quite ready for reviews...

For the future, the way to indicate that the diff is not ready is to put it in the "Plan changes" state

osa1 updated this revision to Diff 8262.Jul 19 2016, 11:00 AM
osa1 edited edge metadata.
  • test suite stuff
  • update user manual
osa1 retitled this revision from Implement SCC_FUNCTION pragma to Support SCC pragmas in declaration context.Jul 19 2016, 11:03 AM
osa1 updated this object.
osa1 edited the test plan for this revision. (Show Details)
osa1 edited edge metadata.
osa1 marked 2 inline comments as done.

@simonmar @bgamari could you review?

@mpickering could you review parser changes?

mpickering requested changes to this revision.Jul 19 2016, 5:16 PM
mpickering added a reviewer: mpickering.

The parser changes look fine (from a brief glance) as long as there are no more reduce/reduce or shift/reduce conflicts.

However, it looks like you need to add a SourceText field to the new AST node. This is for if a user writes something like {-# ScC
then we want to preserve this capitalisation for some tooling.

This revision now requires changes to proceed.Jul 19 2016, 5:16 PM
osa1 updated this revision to Diff 8271.Jul 20 2016, 3:08 AM
osa1 edited edge metadata.
  • add source text
osa1 added a comment.Jul 20 2016, 3:09 AM

@mpickering done.

I'm currently validating (in slow mode). I'll merge this unless someone complains in the meantime.

Nice and small patch.

It would be good to add this feature to the release notes.

compiler/hsSyn/HsBinds.hs
841

off-topic: @mpickering: shouldn't isTypeLSig return True for PatSynSig?

osa1 updated this revision to Diff 8272.Jul 20 2016, 3:56 AM
osa1 edited edge metadata.
  • Document SCCFunSig. Update release notes.
simonmar accepted this revision.Jul 20 2016, 4:02 AM
simonmar edited edge metadata.

Looks fine to me, thanks for doing this!

Please confirm there are no new s/r or r/r conflicts in the grammar.

osa1 added a comment.Jul 20 2016, 4:10 AM

Please confirm there are no new s/r or r/r conflicts in the grammar.

You can't even compile the parser with new conflicts, because of this line:

%expect 36 -- shift/reduce conflicts

It's documented here https://www.haskell.org/happy/doc/html/sec-directives.html#sec-expect

bgamari accepted this revision.Jul 20 2016, 4:14 AM
bgamari edited edge metadata.

Looks good to me. Just one minor documentation thingy.

docs/users_guide/8.2.1-notes.rst
39 ↗(On Diff #8272)

How about,

See the :ref:`scc-pragma` for examples.
osa1 updated this revision to Diff 8273.Jul 20 2016, 4:33 AM
osa1 edited edge metadata.
  • user manual ref
This revision was automatically updated to reflect the committed changes.
mpickering added inline comments.Jul 20 2016, 7:03 AM
compiler/hsSyn/HsBinds.hs
841

I don't know what this function is used for. Maybe it should.