Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prim evaluation fails on undefined arguments #1297

Closed
martijnbastiaan opened this issue Apr 28, 2020 · 2 comments · Fixed by #1299
Closed

Prim evaluation fails on undefined arguments #1297

martijnbastiaan opened this issue Apr 28, 2020 · 2 comments · Fixed by #1299
Assignees
Labels

Comments

@martijnbastiaan
Copy link
Member

The following fails to compile:

import Clash.Prelude
import Numeric.Natural (Natural)

topEntity :: Int
topEntity =
  case compare (toInteger @Natural (0 - 1)) 0 of
    LT -> 2
    _ -> 3

It first emits a warning:

Irreducible constant as case subject: GHC.Integer.Type.compareInteger
  (GHC.Natural.naturalToInteger
     (GHC.Natural.minusNatural 0 1))
  0
Can be reduced to: GHC.Integer.Type.compareInteger
  (GHC.Natural.naturalToInteger
     (Clash.Transformations.undefined
        @GHC.Natural.Natural[3674937295934324782]))
  0

And after a few transformations it crashes:

Test.hs:5:1: error:
    
    Clash.Netlist.BlackBox(284): No blackbox found for: GHC.Integer.Type.compareInteger. Did you forget to include directories containing primitives? You can use '-i/my/prim/dir' to achieve this.
    
    The source location of the error is not exact, only indicative, as it is acquired 
    after optimizations. The actual location of the error can be in a function that is 
    inlined. To prevent inlining of those functions, annotate them with a NOINLINE pragma.
  |
5 | topEntity =
  | ^^^^^^^^^

The expression should be reduced, to undefined. The issue is that the evaluator bails as soon as it sees an undefined value ("Clash.Transformations.undefined") instead of making the result of the primitive undefined too.

@alex-mckenna
Copy link
Contributor

Irreducible constant as case subject: GHC.Integer.Type.compareInteger
(GHC.Natural.naturalToInteger
(GHC.Natural.minusNatural 0 1))
0
Can be reduced to: GHC.Integer.Type.compareInteger
(GHC.Natural.naturalToInteger
(Clash.Transformations.undefined
@GHC.Natural.Natural[3674937295934324782]))
0

I really dislike this warning, it's not very descriptive of what's really happening. If it helps: this warning appears when a case scrutinee is not a constant, but cannot be reduced by the evaluator to a constant (the first snippet of core being what was given, the second being the WHNF of it given by the evaluator).

So as far as this error goes, the evaluator is digging down into the term and reducing the GHC.Natural.minusNatural 0 1 to Clash.Transformations.undefined, but then when it continues unwinding the stack to evaluate GHC.Natural.naturalToInteger, it isn't able to see that

GHC.Natural.naturalToInteger undefined == undefined

It seems that the rule for naturalToInteger starts by reading the argument as a natural literal. If we look at that conversion (line 3504 of Clash.GHC.Evaluator.Primitive) we can see that unless the value matches it returns Nothing for that reduction. I would assume from that point on every delta reduction fails, and just returns the fully applied primop without being able to reduce it.

That should hopefully be all the info you need to work out a fix 😸

@martijnbastiaan
Copy link
Member Author

The issue is slightly more involved: the evaluator can handle GHC.Natural.naturalToInteger Clash.Transformations.undefined (when you put traces in the case alternative for this primitive never fires even when removing all the conditions!), it just can't handle Clash.Transformations.undefined, which it happens to see while it's unwinding the stack. At that point it throws its hands in the air and returns Nothing.

The solution is to consider all primitives that have any undefined arguments as undefined in ghcPrimUnwind and stepTyApp. This is dangerous though, as some primitives do return a result even though one of the arguments is undefined, such as replace_int. We're in luck though, as this is already handled by ghcPrimUnwind. It would be nice to be able to specify what arguments are lazy in primitive definitions (or just store their core expression!) so we can remove the special casing there. But that's out of scope for this issue IMO.

martijnbastiaan added a commit that referenced this issue Apr 30, 2020
Fix #1297

Co-Authored-By: Christiaan Baaij <christiaan.baaij@gmail.com>
martijnbastiaan added a commit that referenced this issue Apr 30, 2020
Fix #1297

Co-Authored-By: Christiaan Baaij <christiaan.baaij@gmail.com>
martijnbastiaan added a commit that referenced this issue Apr 30, 2020
Fix #1297

Co-Authored-By: Christiaan Baaij <christiaan.baaij@gmail.com>
martijnbastiaan added a commit that referenced this issue Apr 30, 2020
Fix #1297

Co-Authored-By: Christiaan Baaij <christiaan.baaij@gmail.com>
martijnbastiaan added a commit that referenced this issue Apr 30, 2020
Fix #1297

Co-Authored-By: Christiaan Baaij <christiaan.baaij@gmail.com>
martijnbastiaan added a commit that referenced this issue Apr 30, 2020
Fix #1297

Co-Authored-By: Christiaan Baaij <christiaan.baaij@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants