Skip to content

Introduce typeclasses for vendored CDC primitives#1246

Open
rslawson wants to merge 1 commit intomainfrom
rs/cdc
Open

Introduce typeclasses for vendored CDC primitives#1246
rslawson wants to merge 1 commit intomainfrom
rs/cdc

Conversation

@rslawson
Copy link
Copy Markdown
Collaborator

What (what did you do)
This should implement (most of?) the desired changes as per #1222.

Why (context, issues, etc.)
See issue for context.

Dear reviewer (anything you'd like the reviewer to pay close attention to?)
I think there's a GHC bug to be found in here. If in Clash.Cores.Xilinx I write

withXilinxI :: forall r. (forall vendor. (VendorCdcImplicit vendor) => r) -> r
withXilinxI = withVendorI @Xilinx

and then replace all withVendorI @Xilinx with withXilinxI calls, I suddenly get a bunch of compilation errors. If it's somehow possible to get this to work though, I'd appreciate a review comment (:

AI disclaimer (heads-up for more than inline autocomplete)

None.

TODO

Cross-out any that do not apply

  • Write (regression) test
  • Update documentation, including docs/
  • Link to existing issue

Copy link
Copy Markdown
Contributor

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this LGTM, the GHC bug is too bad.

I do have three comments:

  1. Instead of making it a big God class, could you make it into singular classes? This would help us if vendor can't supply all the primitives.
  2. I get the appeal of adopting Xilinx's naming scheme, but at the same time I think that scheme doesn't really fit Clash. What do you think of:
  • Single: Bit
  • ArraySingle: IndependentBits
  1. Could you please remove the cdc prefix from the names and design for qualified use? I.e., call sites would look like Cdc.gray.

I'm fairly certain (1) is the way to go, less so of (2), and unswayable on (3) :-)

Comment thread bittide/src/Bittide/Sync.hs Outdated
@rslawson
Copy link
Copy Markdown
Collaborator Author

rslawson commented Apr 16, 2026

Made changes (1) and (3), plus introduced a couple other convenience typeclasses so that it's easier to get the right constraints into scope so that you can call the CDC primitives' functions. I'm a little less enthusiastic about change (2), but I do see the reasoning for it so I did make that change.

Copy link
Copy Markdown
Contributor

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there.. :)

Comment thread bittide/src/Clash/Cores/Xilinx/Xpm/Cdc/Handshake/Extra.hs
Comment thread bittide-extra/src/Clash/Class/Cdc.hs Outdated
Comment thread bittide-extra/src/Clash/Class/Cdc.hs Outdated
Comment thread bittide-extra/src/Clash/Class/Cdc.hs
import Data.Maybe (fromMaybe, isJust, isNothing)
import GHC.Stack (HasCallStack)

class IndependentBits (vendor :: Symbol) where
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add documentation to each of the the functions? They should cover what they synchronize and under what circumstances they can be used. If it helps, this is Xilinx's decision tree:

Image

This one is also very cool, but more out-of-scope:

Image

Comment thread bittide-extra/src/Clash/Class/Cdc.hs
Comment thread bittide/src/Bittide/Counter.hs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants