Skip to content

Simplify KnownDomain#3163

Open
kleinreact wants to merge 2 commits into
masterfrom
simplify-knowndomain
Open

Simplify KnownDomain#3163
kleinreact wants to merge 2 commits into
masterfrom
simplify-knowndomain

Conversation

@kleinreact
Copy link
Copy Markdown
Member

It was irritating me that KnownDomain instances have to define all that redundant boilerplate. Hence, after a small investigation I ended up with the following simplifications implemented by this PR.

The changes basically are a drop-in replacement, except that the associations with the KnownDomain class have slightly changed (only affecting imports of internal modules).;

The most important changes are:

  1. knownDomain is not part of the KnownDomain class anymore, which makes it impossible for users to give a wrong implementation for this function. Note that users could have defined knownDomain = undefined in custom instances before.

  2. KnownConf is not part of the KnownDomain any more, which removes the need for type family applications when access to the fields of KnownDomain dom is required.

    This especially can lead to problems when only access to some particular fields is needed in a constraint signature. With the old definition, GHC simply cannot expect KnownConf dom to be defined as some 'DomainConfiguration a b c d e f by the user potentially leading to type errors. With the update of this PR, KnownConf dom instead is defined to be

    type KnownConf dom = 'DomainConfiguration dom
      (DomainPeriod dom)
      (DomainActiveEdge dom)
      (DomainResetKind dom)
      (DomainInitBehavior dom)
      (DomainResetPolarity dom)

    removing any ambiguity for GHC.

  3. The period, activeEdge, resetKind, initBehavior, and resetPolarity fields are now protected via additional Known... classes. Note that users could have defined something like

    instance KnownDomain Dom48 where
      type KnownConf Dom48 = 'DomainConfiguration Dom48 20833 Any Any Any Any
      ...

    in custom instances before, which now is not possible any more.

  4. The updates simplifies KnownDomain instances now really only needing the relevant values, e.g.,

    instance KnownDomain System where
      type DomainPeriod System        = 10000
      type DomainActiveEdge System    = 'Rising
      type DomainResetKind System     = 'Asynchronous
      type DomainInitBehavior System  = 'Defined
      type DomainResetPolarity System = 'ActiveHigh

As a secondary step, we now technically also could get rid of all the singleton definitions, like SDomainConfiguration, SActiveEdge, etc., just by letting knownDomain directly return a VDomainConfiguration. However, I would keep such an update to be shipped with a separate PR at first.

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

@christiaanb
Copy link
Copy Markdown
Member

Looking at the failing tests, it seems the existing encoding is ingrained in the Clash compiler to figure out things about domain configurations during HDL generation:

  1. let famInstEnvs' = (fst famInstEnvs, modFamInstEnvs)
    allTCInsts = FamInstEnv.famInstEnvElts (fst famInstEnvs')
    ++ FamInstEnv.famInstEnvElts (snd famInstEnvs')
    knownConfs = filter (\x -> "KnownConf" == nameString (FamInstEnv.fi_fam x)) allTCInsts
    fsToText = Text.decodeUtf8 . FastString.bytesFS
    famToDomain = fromMaybe (error "KnownConf: Expected Symbol at LHS of type family")
    . join . fmap (fmap fsToText) . fmap Type.isStrLitTy
    . listToMaybe . FamInstEnv.fi_tys
    famToConf = unpackKnownConf . FamInstEnv.fi_rhs
    knownConfNms = fmap famToDomain knownConfs
    knownConfDs = fmap famToConf knownConfs
    knownConfMap = HashMap.fromList (zip knownConfNms knownConfDs)
    return ( allBinders
    , Map.assocs lbClassOps
    , Set.toList lbUnlocatable
    , famInstEnvs'
    , topEntities2
    , toList lbPrims
    , toList reprs1
    , primGuards
    , knownConfMap
    )
    -- | Given a type that represents the RHS of a KnownConf type family instance,
    -- unpack the fields of the DomainConfiguration and make a VDomainConfiguration.
    --
    unpackKnownConf :: Type.Type -> VDomainConfiguration
    unpackKnownConf ty
    | [d,p,ae,rk,ib,rp] <- Type.tyConAppArgs ty
    -- Domain name
    , Just dom <- fmap FastString.unpackFS (Type.isStrLitTy d)
    -- Period
    , Just period <- fmap naturalFromInteger (Type.isNumLitTy p)
    -- Active Edge
    , aeTc <- Type.tyConAppTyCon ae
    , Just aeDc <- TyCon.isPromotedDataCon_maybe aeTc
    , aeNm <- OccName.occNameString $ Name.nameOccName (DataCon.dataConName aeDc)
    -- Reset Kind
    , rkTc <- Type.tyConAppTyCon rk
    , Just rkDc <- TyCon.isPromotedDataCon_maybe rkTc
    , rkNm <- OccName.occNameString $ Name.nameOccName (DataCon.dataConName rkDc)
    -- Init Behavior
    , ibTc <- Type.tyConAppTyCon ib
    , Just ibDc <- TyCon.isPromotedDataCon_maybe ibTc
    , ibNm <- OccName.occNameString $ Name.nameOccName (DataCon.dataConName ibDc)
    -- Reset Polarity
    , rpTc <- Type.tyConAppTyCon rp
    , Just rpDc <- TyCon.isPromotedDataCon_maybe rpTc
    , rpNm <- OccName.occNameString $ Name.nameOccName (DataCon.dataConName rpDc)
    = VDomainConfiguration dom period
    (asActiveEdge aeNm)
    (asResetKind rkNm)
    (asInitBehavior ibNm)
    (asResetPolarity rpNm)
    | otherwise
    = error $ $(curLoc) ++ "Could not unpack domain configuration."
    where
    asActiveEdge :: HasCallStack => String -> ActiveEdge
    asActiveEdge x = fromMaybe (error $ $(curLoc) ++ "Unknown active edge: " ++ show x) (readMaybe x)
    asResetKind :: HasCallStack => String -> ResetKind
    asResetKind x = fromMaybe (error $ $(curLoc) ++ "Unknown reset kind: " ++ show x) (readMaybe x)
    asInitBehavior :: HasCallStack => String -> InitBehavior
    asInitBehavior x = fromMaybe (error $ $(curLoc) ++ "Unknown init behavior: " ++ show x) (readMaybe x)
    asResetPolarity :: HasCallStack => String -> ResetPolarity
    asResetPolarity x = fromMaybe (error $ $(curLoc) ++ "Unknown reset polarity: " ++ show x) (readMaybe x)
  2. (ActiveEdge edgeRequested n) -> do
    let (_, ty, _) = bbInputs b !! n
    domConf <- getDomainConf ty
    case domConf of
    VDomainConfiguration _ _ edgeActual _ _ _ -> pure $
    if edgeRequested == edgeActual then 1 else 0
  3. getDomainConf :: (Backend backend, HasCallStack) => HWType -> State backend VDomainConfiguration
    getDomainConf = generalGetDomainConf domainConfigurations
    generalGetDomainConf
    :: forall m. (Monad m, HasCallStack)
    => (m DomainMap) -- ^ a way to get the `DomainMap`
    -> HWType -> m VDomainConfiguration
    generalGetDomainConf getDomainMap ty = case (snd . stripAttributes . stripVoid) ty of
    KnownDomain dom period activeEdge resetKind initBehavior resetPolarity ->
    pure $ VDomainConfiguration (Data.Text.unpack dom) (fromIntegral period) activeEdge resetKind initBehavior resetPolarity
    Clock dom -> go dom
    ClockN dom -> go dom
    Reset dom -> go dom
    Enable dom -> go dom
    Product _DiffClock _ [Clock dom,_clkN] -> go dom
    t -> error $ "Don't know how to get a Domain out of HWType: " <> show t
    where
    go :: HasCallStack => N.DomainName -> m VDomainConfiguration
    go dom = do
    doms <- getDomainMap
    case HashMap.lookup dom doms of
    Nothing -> error $ "Can't find domain " <> show dom <> ". Please report an issue at https://github.com/clash-lang/clash-compiler/issues."
    Just conf -> pure conf

@rowanG077
Copy link
Copy Markdown
Member

I would like to do this. In the past I tried something like this as well but I never got it working without breaking type inference on existing code. If you want @kleinreact I can take up this PR and see if I can get it to pass all existing tests.

@kleinreact
Copy link
Copy Markdown
Member Author

Thanks for the offer @rowanG077. I am currently evaluating some more improvements that should be applicable here, where I'd prefer to ship them all in a single PR in the end, as this is a quite sensible part of the prelude. I hope to be able to finish my evaluation soon.

@kleinreact kleinreact force-pushed the simplify-knowndomain branch from 425a142 to ed077ab Compare April 7, 2026 15:10
@kleinreact
Copy link
Copy Markdown
Member Author

I am currently evaluating some more improvements that should be applicable here, where I'd prefer to ship them all in a single PR in the end, as this is a quite sensible part of the prelude. I hope to be able to finish my evaluation soon.

You can find the results of my evaluation for further discussion in #3178.

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