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

Unify the two DSLs for RISE into a single one #76

Merged
merged 11 commits into from
Nov 26, 2020
Merged

Conversation

michel-steuwer
Copy link
Member

This PR removes the DSL that helped to construct untyped expressions for which the type has to be inferred explicitly with a call to infer.

The only remaining DSL uses the ToBeTyped class to construct expressions for which the types are inferred automatically on conversion to the Expr class.

This PR also introduces an import distinction in our interpretation of type annotations:

  • TypeAnnotations (written e :: t) add a constraint that has to be resolved in type inference
  • TypeAssertaions (written e !: t) insist that the type inferred for e must be made compatible with t, that is, the constraint solving only goes one way, type variables in t will not be instantiated during constraint solving, only the once in e.t.

The TypeAsserations are used during rewriting, when we know already what the expected type of the replacement must be and we assert that the inferred type of the expression must be made compatible with the asserted type.

By switching the DSLs this PR also rolls out syntax changes introduced in #70.

Copy link
Member

@Bastacyclop Bastacyclop left a comment

Choose a reason for hiding this comment

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

How does one decide between using :: or !:, and what issues arise when using the wrong one?
I think that we need to discuss equality more so that we have one or if necessary multiple clear definitions. This can be resolved in another PR.

src/main/scala/apps/acoustic3D.scala Show resolved Hide resolved
src/main/scala/apps/cameraPipe.scala Show resolved Hide resolved
src/main/scala/apps/mm.scala Outdated Show resolved Hide resolved
src/main/scala/apps/sgemm.scala Outdated Show resolved Hide resolved
src/main/scala/rise/core/DSL/HighLevelConstructs.scala Outdated Show resolved Hide resolved
src/main/scala/rise/core/DSL/package.scala Show resolved Hide resolved
def untypedTopLevel[T <: Expr](d: ToBeTyped[T]): ToBeTyped[TopLevel] =
toBeTyped(topLevel(toExpr(d)))

def eraseTypeFromExpr[T <: Expr](e: T): T =
Copy link
Member

Choose a reason for hiding this comment

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

maybe this should be private, or contain the word unsafe?

val constraints = mutable.ArrayBuffer[Constraint]()
val (typed_e, ftvSubs) = constrainTypes(e, constraints, mutable.Map())
val solution = unfreeze(ftvSubs, Constraint.solve(constraints.toSeq, Seq())(explDep))
traversal.DepthFirstLocalResult(typed_e, Visitor(solution))
Copy link
Member

@Bastacyclop Bastacyclop Nov 2, 2020

Choose a reason for hiding this comment

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

will this DSL inference perform something like printTypeAndTypeHoles?

printTypesAndTypeHoles(r)

Copy link
Member

@Bastacyclop Bastacyclop Nov 2, 2020

Choose a reason for hiding this comment

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

does this DSL enforce unique names for type inference and DPIA?

val unique_e = uniqueNames.enforce(e)

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not know the relationship to printTypeAndTypeHoles (neither do I understand what is done there).

We are not enforcing unique names in the type inference process.
Substitution is no longer relying on unique names.

Copy link
Member

Choose a reason for hiding this comment

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

printTypeAndTypeHoles will handle debug primitives:

  • print the types of the printType primitives
  • print the types of the typeHole primitives, throwing an error if a type hole is found (because a type hole makes the expression incomplete)

I think that we should add this to the new DSL type inference (while avoiding excessive prints), or alternatively move this code to fromRise on the transition to DPIA.

Copy link
Member

Choose a reason for hiding this comment

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

About unique names, I think that we still require unique names on the DPIA side, so we might want to move the enforce to fromRise if the type inference does not require it anymore.

src/test/scala/rise/elevate/movement.scala Outdated Show resolved Hide resolved
val bnfExpr = normalize(ev).apply(betaReduction)(expr).get
// TODO: consider if we can remove this going forward
val exprWithUniqueNames = uniqueNames.enforce(expr)
val bnfExpr = normalize(ev).apply(betaReduction)(exprWithUniqueNames).get
Copy link
Member

Choose a reason for hiding this comment

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

beta reduction can break unique names ((fun x. x x) (fun y. y) --> (fun y. y) (fun y. y)), maybe we should enforce unique names after normalization.

Michel Steuwer and others added 2 commits November 18, 2020 10:20
This should be revisited in a separate PR.
@michel-steuwer
Copy link
Member Author

I have delayed the implementation of the enforcement of unique names and suggest we accept this PR without it and I will look at this in a separate PR. I hope that's fine with you @Bastacyclop.

Copy link
Member

@Bastacyclop Bastacyclop left a comment

Choose a reason for hiding this comment

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

That's reasonable, please open an issue for the unique names question and the printing.

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