Skip to content

Add HasType / InferType classes - #1915

Merged
alex-mckenna merged 1 commit into
masterfrom
core-hastype-class
Sep 2, 2021
Merged

Add HasType / InferType classes#1915
alex-mckenna merged 1 commit into
masterfrom
core-hastype-class

Conversation

@alex-mckenna

@alex-mckenna alex-mckenna commented Aug 31, 2021

Copy link
Copy Markdown
Contributor

This PR introduces some new classes for inspecting things in Clash core:

class HasType a where
  coreTypeOf :: a -> Type

class InferType a where
  inferCoreTypeOf :: TyConMap -> a -> Type

These functions can then be used to extract type information from any typed thing that exists in core.

Still TODO:

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

Comment on lines +64 to +74
| let (primArgs, primResTy) = splitFunForallTy (primType pr)
, TyConApp tupTcNm tupArgs <- tyView primResTy
, Text.isInfixOf "GHC.Tuple.(" (nameOcc tupTcNm)
-> mkPolyFunTy primResTy (primArgs <> fmap Right tupArgs)

| otherwise
-> error "PrimInfo.coreTypeOf: MultiResult primitive without tuple type"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This really needs a note here to explain why it behaves as it does, e.g. the rationale behind the old multiPrimType that this replaces

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated: the doc in Clash.Normalize.Transformation.MultiPrim was extended and this now points to that.

Comment thread clash-lib/src/Clash/Core/HasType.hs Outdated
Comment on lines +119 to +123
_ -> error $ unlines ["applyTypeToArgs:"
,"Expression: " ++ showPpr e
,"Type: " ++ showPpr opTy
,"Args: " ++ unlines (map (either showPpr showPpr) args)
]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

While this is being moved it might be nice to neaten up the error message

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated: needs to be checked by someone else

Comment thread clash-lib/src/Clash/Core/HasType.hs Outdated
Comment on lines +133 to +141
piResultTy
:: HasCallStack
=> TyConMap
-> Type
-> Type
-> Type
piResultTy m ty arg = case piResultTyMaybe m ty arg of
Just res -> res
Nothing -> pprPanic "piResultTy" (ppr ty <> line <> ppr arg)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wonder if this can just be replaced with piResultTy tcm ty arg = piResultTys tcm ty [arg] and the piResultTyMaybe function then removed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated: needs to be checked by someone else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I mean, if we don't use piResultTyMaybe anywhere any longer, we might as well remove it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't, it was more of a maybe this is some perf trick that I'm just not seeing

@alex-mckenna
alex-mckenna force-pushed the core-hastype-class branch 2 times, most recently from d710111 to 804f7de Compare September 1, 2021 09:24
Comment thread clash-lib/src/Clash/Core/HasType.hs Outdated
Comment thread clash-lib/src/Clash/Core/HasType.hs
Comment thread clash-lib/src/Clash/Core/HasType.hs Outdated

@leonschoorl leonschoorl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice use of typeclasses

Comment thread clash-ghc/src-ghc/Clash/GHC/PartialEval/Eval.hs
@alex-mckenna
alex-mckenna merged commit bdc6e75 into master Sep 2, 2021
@alex-mckenna
alex-mckenna deleted the core-hastype-class branch September 2, 2021 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants