Tailcall ir by matthewhammer · Pull Request #133 · caffeinelabs/motoko

matthewhammer · 2019-01-31T21:51:59Z

Refactor tailcall module to use Ir in place of Syntax.

No apparent issues, except for one question:
See XXX question about sharing flag, and what to do when we don't have it in Ir, in the FuncD case.

This reverts commit 061d24b.

…axops-ir

crusso · 2019-01-31T22:29:43Z

src/tailcall_ir.ml

+    ,
+      env
+(* 
+  XXX: What happens to this special case?  


I think here and in the previous case you'll need to look at the calling convention (cc) instead. The sharing word is a bit of misnomer, I think, but we should just stick with it for now.

That call_conv_of_typ function should reveal the relationship...

Ok, that's very helpful. I'll try to fix this now...

I think I was able to figure it out. See the latest version.

crusso · 2019-01-31T22:32:11Z

src/tailcall_ir.ml

+
+For each function `f` whose `body[...]` has at least one self tailcall to `f<Ts>(es)`, apply the transformation:
+```
+    func f<Ts>(pat) = body[f<Ts>(es)+]


BTW, does this make sense to you? Just checking my own sanity here...

Yes, it does. I read that earlier today was able to suss out what it was saying, and why.

Having said that, it was helpful to have talked with you this morning about the meaning of labels and break, for doing these "control effects" without a stack.

crusso · 2019-01-31T22:34:19Z

src/syntaxops_ir.ml

+     }
+  | _ -> failwith "projE"
+
+(*


Indeed, I believe its redundant in Syntax too. I've got a TODO to remove it at some point...

crusso · 2019-01-31T22:37:36Z

src/syntaxops_ir.ml

+let ( -*- ) exp1 exp2 =
+  match exp1.note.S.note_typ with
+  | T.Func(_, _, [], ts1, ts2) ->
+(* for debugging bad applications, imprecisely


This was a poor man's type check - now that we have check_It, we can probably nuke the commented code too.

Cool, done.

crusso · 2019-01-31T22:38:50Z

src/tailcall_ir.ml

@@ -0,0 +1,305 @@
+(* open Syntax *)
+open Ir
+open Source


Do we still reference Source? If no, nuke (but maye we do for Syntax.mut or something..

Yes: The fields it and note are used in this module; the it field is used somewhat extensively.

But, to avoid namespace pollution, I'll make explicit module projections for each use.

Source /= Syntax. The former just provides the phrase type constructor, the latter the ActorScript source AST.

crusso · 2019-01-31T22:40:05Z

src/syntaxops_ir.ml

+
+let rec typ_decs decs =
+  match decs with
+     | [] -> T.unit


probably my own code, but align cases with match. Actually, typ_decs might even be dead (try grepping for it in src).

It's used once, in this file, just below in blockE. I can move it inside there, as a local function?

crusso · 2019-01-31T22:45:08Z

src/syntaxops_ir.ml

+  | T.Tup ts ->
+     {it = ProjE(e,n);
+      note = {S.note_typ = List.nth ts n;
+              S.note_eff = T.Triv};


T.Triv should be eff e, I think. My mistake, I'm sure. Though the effects don't really matter after the await.ml pass, it's better to get them right. Wonder if any of the others effect annotations might be wrong...

How would we check for this wrongness in the future?

Could it be part of the IR type checking that we do to sanity check each pass?

crusso · 2019-01-31T22:50:47Z

src/pipeline.ml

+
 (* Diagnostics *)

 let phase heading name =


Have you tried replacing the existing tco with the new one? You'd have to move desugar up just before tco and then call the new version of tco.

Not yet. How would I notice problems? Some regression tests would/might fail?

crusso · 2019-02-05T16:57:26Z

I'm closing the PR since it was incorporated into async-ir

## Changelog for candid: Branch: Commits: [dfinity/candid@03773cc2...f838407c](dfinity/candid@03773cc...f838407) * [`a6a413dd`](dfinity/candid@a6a413d) Implement CandidType for std::time::Duration ([dfinity/candid⁠#133](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/133)) * [`f838407c`](dfinity/candid@f838407) release ([dfinity/candid⁠#136](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/136))

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@2c9f2ea8...52736f33](dfinity/ic-hs@2c9f2ea...52736f3) * [`8f47bec4`](dfinity/ic-hs@8f47bec) provisional_create_canister_with_cycles takes an optional specified_id ([dfinity/ic-hs⁠#126](https://github.com/dfinity/ic-hs/issues/126)) * [`af73f215`](dfinity/ic-hs@af73f21) Implement ic0.mint_cycles in ic-hs ([dfinity/ic-hs⁠#127](https://github.com/dfinity/ic-hs/issues/127)) * [`cb9b35d6`](dfinity/ic-hs@cb9b35d) Support requests to a subnet ID ([dfinity/ic-hs⁠#128](https://github.com/dfinity/ic-hs/issues/128)) * [`4cd1c3d3`](dfinity/ic-hs@4cd1c3d) charge canister http_request based on target subnet ([dfinity/ic-hs⁠#130](https://github.com/dfinity/ic-hs/issues/130)) * [`5f07313f`](dfinity/ic-hs@5f07313) replace base64 by ascii endpoint in canister http tests ([dfinity/ic-hs⁠#131](https://github.com/dfinity/ic-hs/issues/131)) * [`9607be0c`](dfinity/ic-hs@9607be0) omit ic-ref binary in ic-ref-test artifact ([dfinity/ic-hs⁠#132](https://github.com/dfinity/ic-hs/issues/132)) * [`67029cb8`](dfinity/ic-hs@67029cb) RUN-509: Enable Canister Timers in pre-upgrade ([dfinity/ic-hs⁠#134](https://github.com/dfinity/ic-hs/issues/134)) * [`52736f33`](dfinity/ic-hs@52736f3) add test for timer in post-upgrade on stopped and stopping canister ([dfinity/ic-hs⁠#133](https://github.com/dfinity/ic-hs/issues/133))

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@2c9f2ea8...ed328b87](dfinity/ic-hs@2c9f2ea...ed328b8) * [`8f47bec4`](dfinity/ic-hs@8f47bec) provisional_create_canister_with_cycles takes an optional specified_id ([dfinity/ic-hs⁠#126](https://github.com/dfinity/ic-hs/issues/126)) * [`af73f215`](dfinity/ic-hs@af73f21) Implement ic0.mint_cycles in ic-hs ([dfinity/ic-hs⁠#127](https://github.com/dfinity/ic-hs/issues/127)) * [`cb9b35d6`](dfinity/ic-hs@cb9b35d) Support requests to a subnet ID ([dfinity/ic-hs⁠#128](https://github.com/dfinity/ic-hs/issues/128)) * [`4cd1c3d3`](dfinity/ic-hs@4cd1c3d) charge canister http_request based on target subnet ([dfinity/ic-hs⁠#130](https://github.com/dfinity/ic-hs/issues/130)) * [`5f07313f`](dfinity/ic-hs@5f07313) replace base64 by ascii endpoint in canister http tests ([dfinity/ic-hs⁠#131](https://github.com/dfinity/ic-hs/issues/131)) * [`9607be0c`](dfinity/ic-hs@9607be0) omit ic-ref binary in ic-ref-test artifact ([dfinity/ic-hs⁠#132](https://github.com/dfinity/ic-hs/issues/132)) * [`67029cb8`](dfinity/ic-hs@67029cb) RUN-509: Enable Canister Timers in pre-upgrade ([dfinity/ic-hs⁠#134](https://github.com/dfinity/ic-hs/issues/134)) * [`52736f33`](dfinity/ic-hs@52736f3) add test for timer in post-upgrade on stopped and stopping canister ([dfinity/ic-hs⁠#133](https://github.com/dfinity/ic-hs/issues/133)) * [`ed328b87`](dfinity/ic-hs@ed328b8) implement canister range checks in ic-ref-test ([dfinity/ic-hs⁠#135](https://github.com/dfinity/ic-hs/issues/135))

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@2c9f2ea8...e29764e9](dfinity/ic-hs@2c9f2ea...e29764e) * [`8f47bec4`](dfinity/ic-hs@8f47bec) provisional_create_canister_with_cycles takes an optional specified_id ([dfinity/ic-hs⁠#126](https://github.com/dfinity/ic-hs/issues/126)) * [`af73f215`](dfinity/ic-hs@af73f21) Implement ic0.mint_cycles in ic-hs ([dfinity/ic-hs⁠#127](https://github.com/dfinity/ic-hs/issues/127)) * [`cb9b35d6`](dfinity/ic-hs@cb9b35d) Support requests to a subnet ID ([dfinity/ic-hs⁠#128](https://github.com/dfinity/ic-hs/issues/128)) * [`4cd1c3d3`](dfinity/ic-hs@4cd1c3d) charge canister http_request based on target subnet ([dfinity/ic-hs⁠#130](https://github.com/dfinity/ic-hs/issues/130)) * [`5f07313f`](dfinity/ic-hs@5f07313) replace base64 by ascii endpoint in canister http tests ([dfinity/ic-hs⁠#131](https://github.com/dfinity/ic-hs/issues/131)) * [`9607be0c`](dfinity/ic-hs@9607be0) omit ic-ref binary in ic-ref-test artifact ([dfinity/ic-hs⁠#132](https://github.com/dfinity/ic-hs/issues/132)) * [`67029cb8`](dfinity/ic-hs@67029cb) RUN-509: Enable Canister Timers in pre-upgrade ([dfinity/ic-hs⁠#134](https://github.com/dfinity/ic-hs/issues/134)) * [`52736f33`](dfinity/ic-hs@52736f3) add test for timer in post-upgrade on stopped and stopping canister ([dfinity/ic-hs⁠#133](https://github.com/dfinity/ic-hs/issues/133)) * [`ed328b87`](dfinity/ic-hs@ed328b8) implement canister range checks in ic-ref-test ([dfinity/ic-hs⁠#135](https://github.com/dfinity/ic-hs/issues/135)) * [`e29764e9`](dfinity/ic-hs@e29764e) fix conditions on read_state requests ([dfinity/ic-hs⁠#136](https://github.com/dfinity/ic-hs/issues/136))

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@2c9f2ea8...ebedfe6e](dfinity/ic-hs@2c9f2ea...ebedfe6) * [`8f47bec4`](dfinity/ic-hs@8f47bec) provisional_create_canister_with_cycles takes an optional specified_id ([dfinity/ic-hs⁠#126](https://github.com/dfinity/ic-hs/issues/126)) * [`af73f215`](dfinity/ic-hs@af73f21) Implement ic0.mint_cycles in ic-hs ([dfinity/ic-hs⁠#127](https://github.com/dfinity/ic-hs/issues/127)) * [`cb9b35d6`](dfinity/ic-hs@cb9b35d) Support requests to a subnet ID ([dfinity/ic-hs⁠#128](https://github.com/dfinity/ic-hs/issues/128)) * [`4cd1c3d3`](dfinity/ic-hs@4cd1c3d) charge canister http_request based on target subnet ([dfinity/ic-hs⁠#130](https://github.com/dfinity/ic-hs/issues/130)) * [`5f07313f`](dfinity/ic-hs@5f07313) replace base64 by ascii endpoint in canister http tests ([dfinity/ic-hs⁠#131](https://github.com/dfinity/ic-hs/issues/131)) * [`9607be0c`](dfinity/ic-hs@9607be0) omit ic-ref binary in ic-ref-test artifact ([dfinity/ic-hs⁠#132](https://github.com/dfinity/ic-hs/issues/132)) * [`67029cb8`](dfinity/ic-hs@67029cb) RUN-509: Enable Canister Timers in pre-upgrade ([dfinity/ic-hs⁠#134](https://github.com/dfinity/ic-hs/issues/134)) * [`52736f33`](dfinity/ic-hs@52736f3) add test for timer in post-upgrade on stopped and stopping canister ([dfinity/ic-hs⁠#133](https://github.com/dfinity/ic-hs/issues/133)) * [`ed328b87`](dfinity/ic-hs@ed328b8) implement canister range checks in ic-ref-test ([dfinity/ic-hs⁠#135](https://github.com/dfinity/ic-hs/issues/135)) * [`e29764e9`](dfinity/ic-hs@e29764e) fix conditions on read_state requests ([dfinity/ic-hs⁠#136](https://github.com/dfinity/ic-hs/issues/136)) * [`ebedfe6e`](dfinity/ic-hs@ebedfe6) make nonexisting canister id in the subnet's ranges ([dfinity/ic-hs⁠#137](https://github.com/dfinity/ic-hs/issues/137))

matthewhammer added 11 commits January 30, 2019 16:00

starting on IR-based refactoring of existing syntaxops module

48ce023

starting on IR-based refactoring of existing syntaxops module

aa4e910

broken state: marked all places that need edits

4936642

still broken, but fewer broken parts, hopefully

503efae

fixed remaining questions with Claudio's help

4b25aee

minor; avoid warning

061d24b

revert change to .gitignore

0b6a586

Revert "minor; avoid warning"

7d7ed49

This reverts commit 061d24b.

Merge branch 'master' of github.com:dfinity-lab/actorscript into synt…

d924f11

…axops-ir

fix 'unused module' warning

d567333

refactor tailcall module to use Ir in place of Syntax

40a5606

matthewhammer requested a review from crusso January 31, 2019 21:51

minor: fix confusing typo in note

7aa8825

crusso reviewed Jan 31, 2019

View reviewed changes

matthewhammer added 2 commits January 31, 2019 16:26

use Claudio's review: hints and feedback

26072a4

use Claudio's review: hints and feedback, continued.

c7bf4e4

crusso closed this Feb 5, 2019

crusso deleted the tailcall-ir branch February 5, 2019 16:57

Conversation

matthewhammer commented Jan 31, 2019

Uh oh!

crusso Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewhammer Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso commented Feb 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

crusso Jan 31, 2019 •

edited

Loading

matthewhammer Jan 31, 2019 •

edited

Loading

crusso Jan 31, 2019 •

edited

Loading

crusso Jan 31, 2019 •

edited

Loading