search

clash-lang / clash-protocols

a battery-included library for dataflow protocols
Other
19 stars 7 forks source link

Change Fwd of Df to Maybe instead of Data #89

Open t-wallet opened 4 months ago

t-wallet commented 4 months ago

Also fixed some linter warnings. HLS was very useful for this job, but it was not configured in the nix shell. I'd like to hear your opinions on adding HLS to shell.nix? We used this in the clash-ethernet repo and it worked pretty well with vscode.

rowanG077 commented 4 months ago

Don't forget to update the readme.

martijnbastiaan commented 4 months ago

HLS was very useful for this job, but it was not configured in the nix shell. I'd like to hear your opinions on adding HLS to shell.nix?

Go for it. I never got it working within Nix though, do you need to start vscode in a specific way?

t-wallet commented 4 months ago

HLS was very useful for this job, but it was not configured in the nix shell. I'd like to hear your opinions on adding HLS to shell.nix?

Go for it. I never got it working within Nix though, do you need to start vscode in a specific way?

Yes, you need to start vscode from the terminal while in nix-shell (and have the vscode hls extension of course).

martijnbastiaan commented 3 months ago

Could you check whether this breaks underscore notation in the circuit plugin?

t-wallet commented 3 months ago

Could you check whether this breaks underscore notation in the circuit plugin?

Tests.Protocols.Plugin still compiles, so I don't think it breaks

martijnbastiaan commented 3 months ago

Ah of course, that's because you did Data -> Maybe, but not Ack -> Bool. Makes sense, sorry for the noise.

christiaanb commented 3 months ago

Do we have some benchmarks that show there’s no space leaks, or at least no simulation performance degradation from this change?

The issue that is closed by this PR just says that “Maybe is lazy, but that’s not a problem any longer”. So what was the original problem that is no longer there?

I guess a benefit of this change is that we no longer have an unlawful Applicative Data instance.

martijnbastiaan commented 3 months ago

The issue that is closed by this PR just says that “Maybe is lazy, but that’s not a problem any longer”. So what was the original problem that is no longer there?

This was more based on a gut feeling back then, after having experienced many space leaks at a client. The idea was that it doesn't make much sense to have an X-value when you have a Data, hence making the seqX in Signal's fmap/app actually do something / do more.

t-wallet commented 3 months ago

Do we have some benchmarks that show there’s no space leaks, or at least no simulation performance degradation from this change?

The issue that is closed by this PR just says that “Maybe is lazy, but that’s not a problem any longer”. So what was the original problem that is no longer there?

I guess a benefit of this change is that we no longer have an unlawful Applicative Data instance.

There was no huge difference in the runtime of the Df tests (actually, the strict version ran 5% slower on average). The point of this PR is to make things uniform, right now we use Data for Df and Maybe for PacketStream and Axi4Stream. I'm also open to doing things the other way around, i.e. using the strict version for every protocol.