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

Behavior different with newest version of Cλash (and how to test via downgrade)? #88

Closed
wyager opened this issue Oct 19, 2015 · 9 comments

Comments

Projects
None yet
2 participants
@wyager
Copy link

commented Oct 19, 2015

All of a sudden, my compiled verilog code stopped working as expected (although the binary version still works as expected). That is to say, the compiled results are now different from the simulated results (using iverilog). I did just update to the newest version of Cλash, and nothing else stands out as having changed. However, I would like to rule this out. How can I downgrade to before Cλash-prelude 0.10?

On hackage, it looks like the constraints are such that a downgrade of clash-ghc won't require older versions of e.g. clash-prelude, which means that, if there's a problem there, this won't fix it. What's the best approach to get my Cλash to the way it was before the most recent updates?

@wyager

This comment has been minimized.

Copy link
Author

commented Oct 19, 2015

OK, so I successfully downgraded. Probably not the best way; I just cabal-uninstalled everything clash-* and then re-installed the minimum version required for clash-ghc < 0.6. So now I'm at the version released before October 3, 2015.

That fixed the issue.

As far as I can tell, this is bad. It means that the behavior of the compiled verilog changed across versions and broke something. The only code changes were replacing <: with :< and sscanl with postscanl. The behavior of these is supposed to be the same, yes?

For now, I'm going to keep using the old version. How can I help you debug this? I don't actually know what's wrong, just that it broke something.

@wyager

This comment has been minimized.

Copy link
Author

commented Oct 20, 2015

I installed the new version of Cλash in a sandbox. I took the exact same code and generated verilog from it with version 0.5.15 (using clash-lib, version: 0.5.13) and version 0.6.1 (using clash-lib, version: 0.6.1).

The verilog files generated by 0.5.15 work, while the ones from 0.6.1 do not. It looks like the primary difference (in terms of files generated) is that 0.6.1 managed to elide a bunch of specF files, so that's cool. I'll try and diff some of the other files, but I'm not hopeful that they'll be structurally similar enough to tell what changed.

Also, something interesting: if I run the sandboxed clash as ./cabal-sandbox/bin/clash --verilog CPU/Adapter.hs, it uses the old version of CLaSH.Prelude (with <: instead of :<). This is fixed by running under a bash session spawned by cabal exec bash. However, it still produces erroneous verilog.

@christiaanb

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2015

Sorry for all the problems with downgrading... Indeed, I should fix my upper bounds on my own packages so that downgrading is not as difficult as it is now.

When you say it broke, you mean that it generated syntactically correct Verilog, but that the Verilog simulation results differ from the Haskell/CLaSH simulation results, correct? Or is it that the generated verilog won't even compile any more?

If it is the first, then there's something wrong with one of the new transformations that I introduced, in which case I would really like to have the Verilog testbench (at least, I assume you're having problems building your Lambda16 project for which I couldn't find the Verilog testbench).

@christiaanb

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2015

OK, It seems to be what I was dreading: Both the VHDL and Verilog code are syntactically correct... as in GHDL and iverilog accept the code. Which means you did indeed mean that the compiler is not preserving the behaviour of the Haskell description, which is bad.

I would really like to have your Verilog testbench (I couldn't find it in your Lambda16 project) so I can run a git bisect to find the offending commit and fix it ASAP.

@wyager

This comment has been minimized.

Copy link
Author

commented Oct 20, 2015

Email sent! The testbench doesn't belong to me, so I didn't put it online publicly.

@christiaanb

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2015

Thank you. I will start working on it right away when I get back in the office tomorrow morning.

@christiaanb

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2015

OK, it took some time to find the bug, but I fixed it now. Next time I'll be sure to test new transformations more thoroughly before putting a release on hackage.

@christiaanb

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2015

Also, thanks a lot for giving me the test bench, it was instrumental in finding the bug. I hope to do a bugfix release today.

@wyager

This comment has been minimized.

Copy link
Author

commented Oct 21, 2015

Yay! No problem; glad I could help.

christiaanb added a commit that referenced this issue Oct 22, 2015

DEC: extract disjoint expresions from alternatives before the subject
In a rush to fix issue #88, i first checked for disjoin
expressions in the subject of a case-expression, before checking
the alternatives of a case-expression. Any expression seen in
the subject, would considered not to be disjoint from the
expression in the alternatives.

However, given a case-expression of the form:

> case (f ..) of A -> (f ..); B -> (f ..); C -> (f ..)

I would not be able to lift out the 'f's in the alternatives
because they are not disjoint from the 'f' in the subject.

So, by checking the alternatives first, I can lift out all the
'f's in the alternatives, but leave the 'f' in the subject
(because it is not disjoint from the 'f's in the alternatives).
Hence DEC now has more oppertunities to make the circuit smaller
while still being correctly implemented.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.