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

IR: Let primitive expressions take manifest arguments #572

Merged
merged 2 commits into from
Jul 17, 2019

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Jul 15, 2019

Triggered by the clean-up necessary post #560 I finally fixe something
that bothered me a long time:

The IR pretends that primitive values are first-class functions, but
they are never compiled that way (and should not, and maybe even cannot,
as in the case of @serialize with a lying polymorophic type), instead
they always have manifest arguments.

So this gives PrimE an exp list.

At this point, there is no fundamental difference anymore between an
unary operation and a primitive with one argument. So unary and binary
operations and relations become just one such primtive operations.
This removes redundancy in IR-to-IR passes.

There is currently an OtherPrim constructor that still takes a string.
Eventually, this should disappear, replaced by an ADT of the actual
prims we support, e.g. a NumericConversion of (Type.prim, Type.prim)
constructor instead of the unstructured "Int->Word32" strings right
now. This would also allow for a nicer backend, as common code is
easiler shared between the many conversions.

Triggered by the clean-up necessary post #560 I finally fixe something
that bothered me a long time:

The IR pretends that primitive values are first-class functions, but
they are never compiled that way (and should not, and maybe even cannot,
as in the case of `@serialize` with a lying polymorophic type), instead
they _always_ have manifest arguments.

So this gives `PrimE` an `exp list`.

At this point, there is no fundamental difference anymore between an
unary operation and a primitive with one argument. So unary and binary
operations and relations become just one such primtive operations.
This removes redundancy in IR-to-IR passes.

There is currently an `OtherPrim` constructor that still takes a string.
Eventually, this should disappear, replaced by an ADT of the actual
prims we support, e.g. a `NumericConversion of (Type.prim, Type.prim)`
constructor instead of the unstructured `"Int->Word32"` strings right
now. This would also allow for a nicer backend, as common code is
easiler shared between the many conversions.
@nomeata nomeata requested review from crusso and ggreif July 15, 2019 16:10
@crusso
Copy link
Contributor

crusso commented Jul 15, 2019

Not a review yet, but it seems that doing it this way means we loose the ability to type-check primitive applications in the IR and just have to trust them. It would be nice if we could check at least some (better all) primitive applications in the IR are well-typed, even those that don't arise from source translation. I wonder if we should attach a perhaps optional type to OtherPrim or, better, an environment mapping primitives to their types. FTR, although I don't think we had any true polymorphic primitives, I believe some of the pseudo primitives (@await) are effectively polymorphic and are used at various instances of their true type within the code. Did you stumble across this?

@nomeata
Copy link
Collaborator Author

nomeata commented Jul 15, 2019

Not a review yet, but it seems that doing it this way means we loose the ability to type-check primitive applications in the IR and just have to trust them.

Quite the opposite, it makes it easier to add custom typing rule for each prim we care about! And polymorphic prims with special typing rules (Serialize, Show) are a main motivation of this.

@crusso
Copy link
Contributor

crusso commented Jul 16, 2019

Not a review yet, but it seems that doing it this way means we loose the ability to type-check primitive applications in the IR and just have to trust them.

Quite the opposite, it makes it easier to add custom typing rule for each prim we care about! And polymorphic prims with special typing rules (Serialize, Show) are a main motivation of this.

But, as it stands, have we not lost some of the typechecking we had? After translation from Syntax to IR, all the 'OtherPrims' are (currently) uncheckable and trusted, or have I missed something?

@nomeata
Copy link
Collaborator Author

nomeata commented Jul 16, 2019

But, as it stands, have we not lost some of the typechecking we had? After translation from Syntax to IR, all the 'OtherPrims' are (currently) uncheckable and trusted, or have I missed something?

They were uncheckable and trusted before as well, so this is not a regression (but a step towards fixing it).

@nomeata
Copy link
Collaborator Author

nomeata commented Jul 16, 2019

More precisely: Previously we blindly trusted the the type annotation on the prim, which had a function type, but could be anything. Now we blindly trust the type annotations of the prim’s arguments, and its result type.

@crusso
Copy link
Contributor

crusso commented Jul 16, 2019

More precisely: Previously we blindly trusted the the type annotation on the prim, which had a function type, but could be anything. Now we blindly trust the type annotations of the prim’s arguments, and its result type.

Is that entirely true? For prelude defined prims, we could rely on all uses in the IR that were derived from source carrying the same annotation we could check against. I think that aspect is still lost and it will be a little easier to construct bad IR terms... Would it not be easy to get the desugarer to insert the required type in the IR term and, perhaps later on replace that hack by a fixed OtherPrim typing function?

@nomeata
Copy link
Collaborator Author

nomeata commented Jul 16, 2019

For prelude defined prims, we could rely on all uses in the IR that were derived from source carrying the same annotation we could check against.

But we can’t rely on that! Passes may change annotations (some of them do traversals into types). Also, some prims work with more than one type (array_len for example).

it will be a little easier to construct bad IR terms...

I think it is just as easy. Why should it be harder now?

But yes, I do want to add type-checking of all prims to check_ir eventually (and maybe refactor the data type to not use strings but enumerate the prims)

@@ -96,6 +95,11 @@ and exp' at note = function
let ty = T.open_ vars ty in
let tys = if cc.Call_conv.n_res = 1 then [ty] else T.as_seq ty in
I.FuncE (name, cc, tbs', args, tys, wrap (exp e))
(* Primitive functions in the prelude have particular shapes *)
| S.CallE ({it=S.AnnotE ({it=S.PrimE p;_},_);_}, _, {it=S.TupE es;_}) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems a shame to discard the type annotation here, when we could use it to check the argument/result typing in IR, but I can live with this...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the typing should not come from an annotation (who knows where it comes from…), but a “real” typing rule for each primitive :)

@crusso
Copy link
Contributor

crusso commented Jul 16, 2019

I'll relent. LGTM.

@nomeata nomeata added the automerge-squash When ready, merge (using squash) label Jul 17, 2019
@mergify mergify bot merged commit 695daf2 into master Jul 17, 2019
@mergify mergify bot deleted the joachim/ir-prim branch July 17, 2019 08:51
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jul 17, 2019
dfinity-bot added a commit that referenced this pull request Jul 4, 2023
## Changelog for motoko-base:
Branch: next-moc
Commits: [dfinity/motoko-base@e5a54883...4df61399](dfinity/motoko-base@e5a5488...4df6139)

* [`26b254ed`](dfinity/motoko-base@26b254e) chore: bump `install-nix-action` to v22 ([dfinity/motoko-base⁠#572](https://togithub.com/dfinity/motoko-base/issues/572))
mergify bot pushed a commit that referenced this pull request Jul 4, 2023
## Changelog for motoko-base:
Branch: next-moc
Commits: [dfinity/motoko-base@e5a54883...4df61399](dfinity/motoko-base@e5a5488...4df6139)

* [`26b254ed`](dfinity/motoko-base@26b254e) chore: bump `install-nix-action` to v22 ([dfinity/motoko-base⁠#572](https://togithub.com/dfinity/motoko-base/issues/572))
dfinity-bot added a commit that referenced this pull request Sep 10, 2024
## Changelog for candid:
Branch: master
Commits: [dfinity/candid@8c20762b...5234523f](dfinity/candid@8c20762...5234523)

* [`5234523f`](dfinity/candid@5234523) add cross-origin header for candid UI ([dfinity/candid⁠#572](https://togithub.com/dfinity/candid/issues/572))
mergify bot pushed a commit that referenced this pull request Sep 10, 2024
## Changelog for candid:
Branch: master
Commits: [dfinity/candid@8c20762b...5234523f](dfinity/candid@8c20762...5234523)

* [`5234523f`](dfinity/candid@5234523) add cross-origin header for candid UI ([dfinity/candid⁠#572](https://togithub.com/dfinity/candid/issues/572))
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.

2 participants