StgLint worth maintaining?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

StgLint worth maintaining?

Ömer Sinan Ağacan
Hi,

I've been looking into some StgLint-related tickets:

- #13994: Found a StgLint problem and fixed, there's another problem
          waiting to be fixed. Both related with the fact that after
          unarisation we lose even more typing information and type
          checks needs to be relaxed.

- #14116: StgLint failed to look through newtypes, and because coercions
          are removed at that point it failed to type check. Solution
          was to relax type checks.

- #5345:  Because `unsafeCoerce# is operationally no-op, and we don't
          have coercions in STG, StgLint can't type check at all. The
          commit message notes:

          > Fundamentally STG Lint is impossible, because unsafeCoerce#
          > can randomise all the types.

          > This patch does a bit of fiddle faddling in StgLint which
          > makes it a bit better, but it's a losing battle.

- #14117: Related with StgLint not keeping up with recent changes (join
          points), because it's not enabled by default in
          tests/validate.

- #14118: Related with the fact that pre- and post-unarise we have
          different invariants in STG. Solution was to add a "unarise"
          parameter and do different checks based on that.

- #14120: Again type checking errors. Commit for #14116 also fixes this.
          The commits compares `typePrimRep`s of types instead of
          comparing actual types (even this is not enough, see #13994).

All this of course took time to debug.

In addition, the new `StgCSE` pass makes transformations that trigger
case alternative checks (and probably some other checks) because
scrutinee and result won't have same types after the transformation
described in `Note [Case 2: CSEing case binders]`.

There's also this comment in StgLint.hs

    WARNING:
    ~~~~~~~~

    This module has suffered bit-rot; it is likely to yield lint errors
    for Stg code that is currently perfectly acceptable for code
    generation.  Solution: don't use it!  (KSW 2000-05).

It seems like it hasn't been used since 2000.

All this suggests that

- Checks related to types are impossible in StgLint. (see e.g. commit
  messages in #5345, #1420, transformations done by unariser and
  StgCSE)

- It's not enabled since 2000, which I think means that it's not
  needed.

This makes me question whether it's worth maintaining. Maybe we should
just remove it.

If we still want to keep we should decide on what it's supposed to do.
Only invariants I can think of are:

- After unarise there should be no unboxed tuple and sum binders.

  unarise is a simple pass and does same thing to all binders, there are
  no tricky cases so I'm not sure if we need to check this.

- Variables should be defined before use. I again don't know if this
  should be checked, could this be useful for StgCSE?

So I think we should do one of these:

1. Remove StgLint.

2. Rewrite it to only check these two and nothing else, enable it in
   validate (and in other build flavours that enable CoreLint).

What do you think? If you think we should keep StgLint, can you think of
any other checks? If we could reach a consensus I'm hoping to update
StgLint (or remove it).

Thanks,

Ömer
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

RE: StgLint worth maintaining?

GHC - devs mailing list
Good summary!  I suggest that you open a ticket with this email as the Description.  Then we can point to it later.

I agree that there is little point in flogging a dead horse.  But there are /some/ invariants, so I vote for
|  2. Rewrite it to only check these two and nothing else, enable it in
|     validate (and in other build flavours that enable CoreLint).
|  

Simon

|  -----Original Message-----
|  From: ghc-devs [mailto:[hidden email]] On Behalf Of Ömer
|  Sinan Agacan
|  Sent: 09 February 2018 08:42
|  To: ghc-devs <[hidden email]>
|  Subject: StgLint worth maintaining?
|  
|  Hi,
|  
|  I've been looking into some StgLint-related tickets:
|  
|  - #13994: Found a StgLint problem and fixed, there's another problem
|            waiting to be fixed. Both related with the fact that after
|            unarisation we lose even more typing information and type
|            checks needs to be relaxed.
|  
|  - #14116: StgLint failed to look through newtypes, and because
|  coercions
|            are removed at that point it failed to type check. Solution
|            was to relax type checks.
|  
|  - #5345:  Because `unsafeCoerce# is operationally no-op, and we don't
|            have coercions in STG, StgLint can't type check at all. The
|            commit message notes:
|  
|            > Fundamentally STG Lint is impossible, because
|  unsafeCoerce#
|            > can randomise all the types.
|  
|            > This patch does a bit of fiddle faddling in StgLint which
|            > makes it a bit better, but it's a losing battle.
|  
|  - #14117: Related with StgLint not keeping up with recent changes
|  (join
|            points), because it's not enabled by default in
|            tests/validate.
|  
|  - #14118: Related with the fact that pre- and post-unarise we have
|            different invariants in STG. Solution was to add a "unarise"
|            parameter and do different checks based on that.
|  
|  - #14120: Again type checking errors. Commit for #14116 also fixes
|  this.
|            The commits compares `typePrimRep`s of types instead of
|            comparing actual types (even this is not enough, see
|  #13994).
|  
|  All this of course took time to debug.
|  
|  In addition, the new `StgCSE` pass makes transformations that trigger
|  case alternative checks (and probably some other checks) because
|  scrutinee and result won't have same types after the transformation
|  described in `Note [Case 2: CSEing case binders]`.
|  
|  There's also this comment in StgLint.hs
|  
|      WARNING:
|      ~~~~~~~~
|  
|      This module has suffered bit-rot; it is likely to yield lint
|  errors
|      for Stg code that is currently perfectly acceptable for code
|      generation.  Solution: don't use it!  (KSW 2000-05).
|  
|  It seems like it hasn't been used since 2000.
|  
|  All this suggests that
|  
|  - Checks related to types are impossible in StgLint. (see e.g. commit
|    messages in #5345, #1420, transformations done by unariser and
|    StgCSE)
|  
|  - It's not enabled since 2000, which I think means that it's not
|    needed.
|  
|  This makes me question whether it's worth maintaining. Maybe we should
|  just remove it.
|  
|  If we still want to keep we should decide on what it's supposed to do.
|  Only invariants I can think of are:
|  
|  - After unarise there should be no unboxed tuple and sum binders.
|  
|    unarise is a simple pass and does same thing to all binders, there
|  are
|    no tricky cases so I'm not sure if we need to check this.
|  
|  - Variables should be defined before use. I again don't know if this
|    should be checked, could this be useful for StgCSE?
|  
|  So I think we should do one of these:
|  
|  1. Remove StgLint.
|  
|  2. Rewrite it to only check these two and nothing else, enable it in
|     validate (and in other build flavours that enable CoreLint).
|  
|  What do you think? If you think we should keep StgLint, can you think
|  of any other checks? If we could reach a consensus I'm hoping to
|  update StgLint (or remove it).
|  
|  Thanks,
|  
|  Ömer
|  _______________________________________________
|  ghc-devs mailing list
|  [hidden email]
|  https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.h
|  askell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
|  devs&data=04%7C01%7Csimonpj%40microsoft.com%7C9d9affa423c84c84a25908d5
|  6f992d87%7Cee3303d7fb734b0c8589bcd847f1c277%7C1%7C0%7C6365376260479856
|  64%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
|  6Ik1haWwifQ%3D%3D%7C-
|  1&sdata=GZ4xMoVQGyQFZxhlODBqMnWoiZrV82pqOn2ZrbvDo4U%3D&reserved=0
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: StgLint worth maintaining?

Ömer Sinan Ağacan
Created #14787 as tracking ticket. Patch is at D4404.

Ömer

2018-02-09 12:22 GMT+03:00 Simon Peyton Jones <[hidden email]>:

> Good summary!  I suggest that you open a ticket with this email as the Description.  Then we can point to it later.
>
> I agree that there is little point in flogging a dead horse.  But there are /some/ invariants, so I vote for
> |  2. Rewrite it to only check these two and nothing else, enable it in
> |     validate (and in other build flavours that enable CoreLint).
> |
>
> Simon
>
> |  -----Original Message-----
> |  From: ghc-devs [mailto:[hidden email]] On Behalf Of Ömer
> |  Sinan Agacan
> |  Sent: 09 February 2018 08:42
> |  To: ghc-devs <[hidden email]>
> |  Subject: StgLint worth maintaining?
> |
> |  Hi,
> |
> |  I've been looking into some StgLint-related tickets:
> |
> |  - #13994: Found a StgLint problem and fixed, there's another problem
> |            waiting to be fixed. Both related with the fact that after
> |            unarisation we lose even more typing information and type
> |            checks needs to be relaxed.
> |
> |  - #14116: StgLint failed to look through newtypes, and because
> |  coercions
> |            are removed at that point it failed to type check. Solution
> |            was to relax type checks.
> |
> |  - #5345:  Because `unsafeCoerce# is operationally no-op, and we don't
> |            have coercions in STG, StgLint can't type check at all. The
> |            commit message notes:
> |
> |            > Fundamentally STG Lint is impossible, because
> |  unsafeCoerce#
> |            > can randomise all the types.
> |
> |            > This patch does a bit of fiddle faddling in StgLint which
> |            > makes it a bit better, but it's a losing battle.
> |
> |  - #14117: Related with StgLint not keeping up with recent changes
> |  (join
> |            points), because it's not enabled by default in
> |            tests/validate.
> |
> |  - #14118: Related with the fact that pre- and post-unarise we have
> |            different invariants in STG. Solution was to add a "unarise"
> |            parameter and do different checks based on that.
> |
> |  - #14120: Again type checking errors. Commit for #14116 also fixes
> |  this.
> |            The commits compares `typePrimRep`s of types instead of
> |            comparing actual types (even this is not enough, see
> |  #13994).
> |
> |  All this of course took time to debug.
> |
> |  In addition, the new `StgCSE` pass makes transformations that trigger
> |  case alternative checks (and probably some other checks) because
> |  scrutinee and result won't have same types after the transformation
> |  described in `Note [Case 2: CSEing case binders]`.
> |
> |  There's also this comment in StgLint.hs
> |
> |      WARNING:
> |      ~~~~~~~~
> |
> |      This module has suffered bit-rot; it is likely to yield lint
> |  errors
> |      for Stg code that is currently perfectly acceptable for code
> |      generation.  Solution: don't use it!  (KSW 2000-05).
> |
> |  It seems like it hasn't been used since 2000.
> |
> |  All this suggests that
> |
> |  - Checks related to types are impossible in StgLint. (see e.g. commit
> |    messages in #5345, #1420, transformations done by unariser and
> |    StgCSE)
> |
> |  - It's not enabled since 2000, which I think means that it's not
> |    needed.
> |
> |  This makes me question whether it's worth maintaining. Maybe we should
> |  just remove it.
> |
> |  If we still want to keep we should decide on what it's supposed to do.
> |  Only invariants I can think of are:
> |
> |  - After unarise there should be no unboxed tuple and sum binders.
> |
> |    unarise is a simple pass and does same thing to all binders, there
> |  are
> |    no tricky cases so I'm not sure if we need to check this.
> |
> |  - Variables should be defined before use. I again don't know if this
> |    should be checked, could this be useful for StgCSE?
> |
> |  So I think we should do one of these:
> |
> |  1. Remove StgLint.
> |
> |  2. Rewrite it to only check these two and nothing else, enable it in
> |     validate (and in other build flavours that enable CoreLint).
> |
> |  What do you think? If you think we should keep StgLint, can you think
> |  of any other checks? If we could reach a consensus I'm hoping to
> |  update StgLint (or remove it).
> |
> |  Thanks,
> |
> |  Ömer
> |  _______________________________________________
> |  ghc-devs mailing list
> |  [hidden email]
> |  https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.h
> |  askell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
> |  devs&data=04%7C01%7Csimonpj%40microsoft.com%7C9d9affa423c84c84a25908d5
> |  6f992d87%7Cee3303d7fb734b0c8589bcd847f1c277%7C1%7C0%7C6365376260479856
> |  64%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
> |  6Ik1haWwifQ%3D%3D%7C-
> |  1&sdata=GZ4xMoVQGyQFZxhlODBqMnWoiZrV82pqOn2ZrbvDo4U%3D&reserved=0
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs