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

Scala rewrite #46

Merged
merged 45 commits into from Feb 6, 2019
Merged

Scala rewrite #46

merged 45 commits into from Feb 6, 2019

Conversation

rachitnigam
Copy link
Member

@rachitnigam rachitnigam commented Jan 31, 2019

Fixes #44 #41 #37 #30. Subsumes #40 #43.

TODO

Copy link
Contributor

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

This is looking good! I'm happy with how things are going in the Scala incantation. (The only drawback is indeed startup time—sbt really is the opposite of ocamlc in that sense!)

Aside from the notes I've left inline during a review, here are a few quick notes:

  • A bit more commenting could really help with medium-term maintainability.
  • I like the new small language-feature test suite!!
  • It looks like the old collection of larger examples (in examples) needs updating to sync with the new implementation.

src/main/scala/TypeCheck.scala Show resolved Hide resolved
src/main/scala/TypeCheck.scala Show resolved Hide resolved
src/main/scala/TypeCheck.scala Show resolved Hide resolved
src/main/scala/TypeCheck.scala Outdated Show resolved Hide resolved
src/main/scala/TypeCheck.scala Show resolved Hide resolved
src/main/scala/Emit.scala Outdated Show resolved Hide resolved
src/main/scala/TypeCheck.scala Show resolved Hide resolved
if (t2 :< t1) (t1, t2, lhs) match {
// Reassignment of static ints upcasts to bit<32>
case (TStaticInt(_), TStaticInt(_), EVar(id)) =>
e2.updateBind(id -> Info(id, TSizedInt(32)))
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to make sure that the number fits into a 32-bit size. And even if not, it could be sensible to put the default integer type into a constant somewhere.

I realize this seems like a low-level detail, but this could actually be a deeper question: we might want to let people specify the default numeric type within a scope. Because numeric widths are so important for accelerator efficiency, people often want to decide the numeric type for an entire design or module at once. That should probably affect (most?) literals as well as the buffers they eventually flow into. Seems sort of tricky.

Copy link
Member Author

Choose a reason for hiding this comment

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

Explicitly typed let allows you to specific the exact type of constants. Is that what you’re talking about?

src/main/scala/TypeCheck.scala Show resolved Hide resolved
src/main/scala/TypeEnv.scala Outdated Show resolved Hide resolved
@rachitnigam
Copy link
Member Author

rachitnigam commented Feb 6, 2019

Re: Examples. They have been moved to src/test/resources and are run with the test suite.

The examples directory is used by Sachille for something, but he can starting migrating things to the same place.

@rachitnigam rachitnigam mentioned this pull request Feb 6, 2019
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.

None yet

2 participants