[WIP, don’t merge] IR pass: Simplify loop forms into a single, simple form by matthewhammer · Pull Request #146 · caffeinelabs/motoko

matthewhammer · 2019-02-05T23:28:55Z

Initially, this pass is the identify function, and this description, to make sure I'm heading the right direction. The three transformation cases mentioned below should each be fairly straightforward, given this set up.

Loop transformations

To make the IR "narrower", this pass rewrites the following forms each to LoopE(e', None), for some appropriate e':

LoopE(_, Some _)
WhileE(_, _)
ForE(_,_,_)

To afford the control flow of these special forms, the new loop body e' introduces labels andBreakEs, as needed.

nomeata · 2019-02-05T23:38:07Z

If the rewrites are simple enough (for example, if they don’t need to do complex rewriting of subexpressions), they can just happen in desugar.ml, and we can remove the constructors from Ir altogether. Like we do for AndE.

matthewhammer · 2019-02-05T23:39:58Z

I wondered about that. Why include them in the IR at all if the goal is to take them out at some point in the pipeline?

One reason could be that this makes the transformation optional, though that reason doesn't sound very compelling (why do this at all, then?).

@crusso ?

matthewhammer · 2019-02-05T23:41:20Z

In any case, if my assumptions about what forms to rewrite, and how to rewrite them, are all correct, it will be easy to move this into desugar once I get it right.

matthewhammer · 2019-02-06T00:32:12Z

Just reread #144

It seems like the intention is to actually do what @nomeata says above, i.e., remove these "extra" forms from the IR, and do these loop-simplification transformations in the existing desugar pass.

So, I'll plan on doing that approach, once I get this working.

nomeata · 2019-02-06T08:11:42Z

I think the guideline of whether you do it in the desugarer (like AndE) or in a separate pass (like AwaitE) is:

Can you do the transformation by looking at the AST node (including type annotations), without knowing anything about the subterms or the context? Do it in the desugarer.
Do you need non-local information? Do it in a separate pass.

…ould be straightforward, given answers to the questions

matthewhammer · 2019-02-06T17:30:59Z

I think the guideline of whether you do it in the desugarer (like AndE) or in a separate pass (like AwaitE) is:
* Can you do the transformation by looking at the AST node (including type annotations), without knowing anything about the subterms or the context? Do it in the desugarer.

* Do you need non-local information? Do it in a separate pass.

OK, thanks for this. I've put some questions, and a sketch of each planned transform, here:
c686ad0

Please have a look, especially for the questions there.

matthewhammer · 2019-02-07T14:11:40Z

I'm going to continue working this today, following @crusso's tips and advice, just above.

src/construct.ml

The benefit is that many passes no longer need to keep a `con_env` around, and many operations in `Type` are now always available, in partiuclar `Type.as_*_sub`, `promote` etc. The downside is that, because of the multi-pass approach in `typing.ml`, the annotation needs to be a `kind ref`. I tried using a `promise`, but it seems that certain things are run more than once in `typing` (the same has caused issues with recoverable error messages before). I am not sure if this is avoidable. If maybe the initialization can be made a bit cleaner in `typing`, then I think this will improve the life of those working on the `Ir` noticably.

the `kind` is already in the `con`, so no need having it here.

so that the choice whether it is a `promise`, or a `ref`, or something else, private to `type.ml`. I tried to make it more promise-like, by trapping when the kind is set a second time to something different, but it seems that `k = k'` can loop.

with a different value. A bit like a promise, but more compatible with `typing.ml` running `infer_dec_typdecs` multiple times. Not sure if it is worth the bother, though.

In `typing.ml`, we replace it with a `con_set`, so that we can still call `avoid`. In `check_ir.ml` we simply drop the field.

it is always the range of `typ_env` anyways. This simplifies a few more functions.

and other improvements cherry-picked from #171 and #146, namely: * add an interface file (and remove code known to be dead) * make the `Check_ir.env` abstract

crusso · 2019-02-15T13:36:21Z

src/construct.ml

+  (* XXX: not sure how to get type info for `next` function...
+     - ...how to do I supply a non-empty con_env for `as_obj_sub` ?
+   *)
+  let _, tfs  = Type.as_obj_sub "next" ty1 in


I think there actually is a named constant for "next" (Construct.nextN), best to use it here and elsewhere

Actually, couldn't you just construct the type of next as " () -> ?(E.typ pat)", (without needing an env) but maybe that's cheating.

@crusso hmm; I cannot use Construct.nextN here, since the code I want to call next (part of the Type module) expects a string, not a name.

You should be able to get the string from the name. Name is just a single constructor type (or at least it was) to prevent accidental confusion between alpha-convertible identifiers and rigid field names in the syntax. Andreas may have renamed Name to Lab(el) recently (but perhaps only in source). Type's probably just use string for field names...

crusso · 2019-02-15T13:42:44Z

src/construct.ml

+let loopWhileE exp1 exp2 =
+  (* loop e1 while e2
+    ~~> label l: loop {
+          let () = e1 ;


let () = e1 -> e1;

Huh? I don't understand this comment; is something wrong in the (source) comment above?

crusso · 2019-02-15T13:43:38Z

src/construct.ml

+          blockE [
+              expD exp1 ;
+              letD id2 exp2 ;
+              expD (ifE id2


why bind to id2? (Perhaps Joachim spotted this one)

In general, I was using ANF to make things "easier" for me;
yesterday, @nomeata mentioned that this was suboptimal for the WASM output, so I'll avoid these intermediate bindings, and will fix this one.

K, but unfortunately, there's a cost, especially with recursion story (though I suspect Joachim optimizes this away now)

src/construct.ml

crusso · 2019-02-15T13:52:53Z

src/construct.mli

 val labelE : Syntax.id -> typ -> exp -> exp
 val loopE: exp -> exp option -> exp

+val whileE' : exp -> exp -> exp'


Could these be whileE : ... -> exp etc, dropping the primes, in keeping with the rest of the API?

You'll need to correctly construct the type (of course) but also the effect too or this will mess up other transformations.

I don't see how to do this "cleanly"; how would you adjust these use cases? (see Simploop):

(* Transformation from loop-while to loop: *) | I.LoopE (e1, Some e2) -> Construct.loopWhileE' (exp e1) (exp e2) (* Transformation from while to loop: *) | I.WhileE (e1, e2) -> Construct.whileE' (exp e1) (exp e2) (* Transformation from for to loop: *) | I.ForE (p, e1, e2) -> Construct.forE' p (exp e1) (exp e2)

True, but I guess here you could just do loopWhile (exp e1) (exp e2).it. I'm pretty sure I do that elsewhere in a few places...

Yes, that's the pattern I was trying to avoid, but I can use it if you have a strong preference.

I don't have a preference myself, I just thought the version projecting out the it seemed slightly more "messy"; but again, I have no strong opinion; I will fix this.

I guess you could provide both in this case... but, in general, the construct functions are there to make it easier to construct (nested) expressions, where its more convenient to have the exp return type for composing subexpressions.

how would you adjust these use cases?

You would have to refactor the surrounding code to expect an exp, not an exp'. But that might be ugly, so maybe having both is fine.

crusso · 2019-02-15T13:56:33Z

src/simploop.ml

+     - Continue isn't special IRCC, it's just a special named label ("continue-" or something like that) introduced by the parser.
+       (the loop(_,None) construct just loops forever, but you can still exit via break).
+
+     - I would indeed just do this desugaring in the parser


Not sure about this comment - I think our intention is to eventually remove the complicated loops from the IR and desugar source loops to ir loops directly in the translator...

I guess once could imagine doing forE loops slight differently, optimizing for each collection type, so perhaps those would be best as a full blown transformation....

Desugaring is actually a bad name for the source to IR translation... desugaring usually means source 2 source...

Even if we don't remove the constructs in the src->ir desugaring, it would be great if we could move the transform up the pipeline so awaitopt.ml and async.ml and tailcall.ml no longer need to consider all those looping constructs. At the moment only the compiler benefits.

I would indeed just do this desugaring in the parser

you (@crusso) wrote that, not me; I pasted it here from a GH conversation we had earlier :)
are you saying that you no longer believe this?

The rest of your comments make sense (e.g., regarding moving up this translation in the pipeline).

Here's a link to the source of the aforementioned quoted text:
#146 (comment)

Ignore my comment - I think Andreas and Joachim didn't want this for better error reporting (and I guess to keep the AST more in line with syntax).

nomeata · 2019-02-25T18:34:48Z

Is it time to un-WIP this and get it merged? Do you want to include it in desugar.ml?

matthewhammer · 2019-02-26T13:07:01Z

Is it time to un-WIP this and get it merged? Do you want to include it in desugar.ml?

I think: Yes, and yes. I can work getting this done today.

matthewhammer · 2019-02-26T14:08:29Z

@nomeata in preparation for doing this today, I moved the transforms into Desugar and removed Simploop from the pipeline. I also removed some uses of ANF in the transform's output. In the process, some other issues arose:

I have a question about one failed attempt to inline ANF; see my code comment within last commit; cc @crusso. Basically, array-gen diverges when I try to aggressively inline the projection of .next for the for-loop simplification. I don't understand why.
I'm getting (new?) errors on the dvm tests, after the first wave of tests all pass.
I tried (perhaps naively) to address the "conflicting files" issue above by rebasing with master; that didn't help, but maybe I did it wrong. I did git pull --rebase origin master, but git wanted me to fix each of those conflicts above by hand, and that seemed wrong; is there a way to avoid this?

nomeata · 2019-02-26T14:28:44Z

I have a question about one failed attempt to inline ANF; see my code comment within last commit;

For those too lazy to search for this comment:
629b881#diff-b62a2a2b6b830d0e342967b88b07871eR472

If you inline letD nxt (dotE exp1 (nameN "next") tnxt) ; you might duplicate the evaluation of exp1. You still need to evaluate and bind exp1, once to not change the semantics. Might that be it?

I tried (perhaps naively) to address the "conflicting files" issue above by rebasing with master; that didn't help, but maybe I did it wrong. I did git pull --rebase origin master, but git wanted me to fix each of those conflicts above by hand, and that seemed wrong; is there a way to avoid this?

Well, if there are conflicts, you have to resolve them. Why do you think they are wrong?

Or is the problem that you have to do them repeatedly, as you rebase your patch series? You don’t really have a “clean, narrative” patch set anyways, so rebasing does not seem to be the right thing. Simply git pull origin master and do a merge.

I'm getting (new?) errors on the dvm tests, after the first wave of tests all pass.

Master is green, so these failures are introduced by your change. Looking at the output, it seem that the IR output has changed in cases where the compiler dumps IR (variable names have changed). If the changes are expected, then run make accept in test to make them part of this PR.

nomeata · 2019-02-26T14:38:28Z

BTW, I just checked what happens if .next changes between loops, and wrote a test case:

let iter = new {
  private var i = 0;
  var next : (() -> ?()) = func () : ?() {
    print("old next: ");
    printInt(i);
    print("\n");
    i += 1;
    next := (func () : ?() {
      print("new next: ");
      printInt(i);
      print("\n");
      i += 1;
      return null;
    });
    return (?());
  }
};

for (x in iter) {
  print("Iter going\n");
};
print("Iter gone\n");

But actually the typing rule for for requires an immutable field next, so this is not an issue.

I don’t think anything is wrong with letD nxt (dotE exp1 (nameN "next") tnxt) btw. You need to bind the evaluation of exp1 anyways, and doing the lookup of next only once is fine.

matthewhammer · 2019-02-26T16:42:25Z

Thanks for all of the feedback and clarifications @nomeata.

You still need to evaluate and bind exp1, once to not change the semantics. Might that be it?

Yep, I did that naively this morning. It was too early. I think that was the issue. Thanks for your investigation, in any case; I'll leave that transform for for alone now, since it seems okay with you.

Or is the problem that you have to do them repeatedly, as you rebase your patch series?

If I understand correctly, then "yes", each time I want to stay in sync with master now on this branch, I have conflicts like this. I was doing rebase because I thought it was no worse than merge, but I frankly don't appreciate the wider implications of either approach. The reason the conflicting files seemed "wrong" to me is that I never directly changed many of these files on my branch. Rather, my branch only affected a few existing modules (e.g., Construct, Pipeline and Desugar, perhaps one or two others), and the other modules (most from that list) were untouched. However, I (naively?) did a rebase earlier in the commit history that made these changes implicitly, I think; I guess that was a mistake, and I should have been doing something else to stay in sync with master, such as merge? Again, I'm confused by the "correct" way to do branches in git while master evolves concurrently. I take from your last message that I should always do merge and never rebase for branches such as this; but please correct me if I'm misinterpreting what you meant.

these failures are introduced by your change. Looking at the output, it seem that the IR output has changed in cases where the compiler dumps IR (variable names have changed)

Yes, I do see one instance of that. However, I also see things like this as well:

--- array-out-of-bounds.dvm-run (expected)
+++ array-out-of-bounds.dvm-run (actual)
@@ -1,2 +1,2 @@
-W, hypervisor: call failed with trap message: Uncaught RuntimeError: unreachable
+W, hypervisor: call failed with trap message: Uncaught RuntimeError: unreachable

This doesn't seem like a simple variable name difference. (am I missing something?). There are several errors like this, with the same unreachable trap message. Did you see those too?

In the meantime, I'm going to try to update DVM to see if that helps.

matthewhammer · 2019-02-26T17:18:03Z

Updates:

doing update-dvm didn't seem to make these traps go away locally.
I did a fresh clone of master, and I get them there, too; so, it's something about my local build and test environment, it seems.

matthewhammer · 2019-02-26T22:17:01Z

The commit history here is a mess, so I made a cleaner PR with a distillation of my changes. We can close this PR.

nomeata · 2019-02-27T10:22:21Z

lots about git

If you feel like it, we can video-chat about the meaning and implications of rebase vs. merge. More efficient than writing about it here.

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@927d8152...6ddff2e4](dfinity/ic-hs@927d815...6ddff2e) * [`54dcdd0c`](dfinity/ic-hs@54dcdd0) .github/workflows/release.yml: only sync github pages on linux ([dfinity/ic-hs⁠#143](https://github.com/dfinity/ic-hs/issues/143)) * [`81ac2e9c`](dfinity/ic-hs@81ac2e9) .github/workflows/release.yml: fix github-pages-deploy-action version ([dfinity/ic-hs⁠#144](https://github.com/dfinity/ic-hs/issues/144)) * [`09a7b445`](dfinity/ic-hs@09a7b44) fix broken link in README ([dfinity/ic-hs⁠#145](https://github.com/dfinity/ic-hs/issues/145)) * [`b393d8cb`](dfinity/ic-hs@b393d8c) .github/workflows/release.yml: skip coverage job on darwin ([dfinity/ic-hs⁠#146](https://github.com/dfinity/ic-hs/issues/146)) * [`55870466`](dfinity/ic-hs@5587046) split haskell modules to reduce memory usage ([dfinity/ic-hs⁠#142](https://github.com/dfinity/ic-hs/issues/142)) * [`6ddff2e4`](dfinity/ic-hs@6ddff2e) add httpbin implementation in Rust, bump nixpkgs, and sync universal_canister with ic monorepo ([dfinity/ic-hs⁠#138](https://github.com/dfinity/ic-hs/issues/138))

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@927d8152...808ba08b](dfinity/ic-hs@927d815...808ba08) * [`54dcdd0c`](dfinity/ic-hs@54dcdd0) .github/workflows/release.yml: only sync github pages on linux ([dfinity/ic-hs⁠#143](https://github.com/dfinity/ic-hs/issues/143)) * [`81ac2e9c`](dfinity/ic-hs@81ac2e9) .github/workflows/release.yml: fix github-pages-deploy-action version ([dfinity/ic-hs⁠#144](https://github.com/dfinity/ic-hs/issues/144)) * [`09a7b445`](dfinity/ic-hs@09a7b44) fix broken link in README ([dfinity/ic-hs⁠#145](https://github.com/dfinity/ic-hs/issues/145)) * [`b393d8cb`](dfinity/ic-hs@b393d8c) .github/workflows/release.yml: skip coverage job on darwin ([dfinity/ic-hs⁠#146](https://github.com/dfinity/ic-hs/issues/146)) * [`55870466`](dfinity/ic-hs@5587046) split haskell modules to reduce memory usage ([dfinity/ic-hs⁠#142](https://github.com/dfinity/ic-hs/issues/142)) * [`6ddff2e4`](dfinity/ic-hs@6ddff2e) add httpbin implementation in Rust, bump nixpkgs, and sync universal_canister with ic monorepo ([dfinity/ic-hs⁠#138](https://github.com/dfinity/ic-hs/issues/138)) * [`808ba08b`](dfinity/ic-hs@808ba08) split Multiple controllers tests ([dfinity/ic-hs⁠#149](https://github.com/dfinity/ic-hs/issues/149))

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@927d8152...bdaaa321](dfinity/ic-hs@927d815...bdaaa32) * [`54dcdd0c`](dfinity/ic-hs@54dcdd0) .github/workflows/release.yml: only sync github pages on linux ([dfinity/ic-hs⁠#143](https://github.com/dfinity/ic-hs/issues/143)) * [`81ac2e9c`](dfinity/ic-hs@81ac2e9) .github/workflows/release.yml: fix github-pages-deploy-action version ([dfinity/ic-hs⁠#144](https://github.com/dfinity/ic-hs/issues/144)) * [`09a7b445`](dfinity/ic-hs@09a7b44) fix broken link in README ([dfinity/ic-hs⁠#145](https://github.com/dfinity/ic-hs/issues/145)) * [`b393d8cb`](dfinity/ic-hs@b393d8c) .github/workflows/release.yml: skip coverage job on darwin ([dfinity/ic-hs⁠#146](https://github.com/dfinity/ic-hs/issues/146)) * [`55870466`](dfinity/ic-hs@5587046) split haskell modules to reduce memory usage ([dfinity/ic-hs⁠#142](https://github.com/dfinity/ic-hs/issues/142)) * [`6ddff2e4`](dfinity/ic-hs@6ddff2e) add httpbin implementation in Rust, bump nixpkgs, and sync universal_canister with ic monorepo ([dfinity/ic-hs⁠#138](https://github.com/dfinity/ic-hs/issues/138)) * [`808ba08b`](dfinity/ic-hs@808ba08) split Multiple controllers tests ([dfinity/ic-hs⁠#149](https://github.com/dfinity/ic-hs/issues/149)) * [`bdaaa321`](dfinity/ic-hs@bdaaa32) bump cachix/install-nix-action to v20 ([dfinity/ic-hs⁠#150](https://github.com/dfinity/ic-hs/issues/150))

initial draft; the pass does nothing at all, yet

1b886cc

matthewhammer requested a review from crusso February 5, 2019 23:28

sanity check: pipeline still passes all tests

7b739f1

nomeata changed the title ~~IR pass: Simplify loop forms into a single, simple form~~ [WIP, don’t merge] IR pass: Simplify loop forms into a single, simple form Feb 6, 2019

notes and questions about the planned transforms; writing them out sh…

c686ad0

…ould be straightforward, given answers to the questions

ninegua mentioned this pull request Feb 6, 2019

Fix array syntax explanation in README #152

Merged

matthewhammer added 5 commits February 8, 2019 04:15

move RHS's of loop transforms, stil pending, into Construct module

df1f3c1

1/3 of the derived loop forms in Construct; triggering WASM test errors

2467678

minor: fix typo in comment

517d236

fix bug that I introduced; another xform case passes all tests

53fbdc1

stuck: how to get the Con env where I need it?

c28bf05

matthewhammer commented Feb 8, 2019

View reviewed changes

src/construct.ml Outdated Show resolved Hide resolved

nomeata added 11 commits February 8, 2019 18:24

Simplify Ir.TypD

0dde544

the `kind` is already in the `con`, so no need having it here.

Make set_kind check that the kind is not set a second time

d5baf2b

with a different value. A bit like a promise, but more compatible with `typing.ml` running `infer_dec_typdecs` multiple times. Not sure if it is worth the bother, though.

Address nit

da7d6ae

Merge branch 'master' into joachim/con-ann

779c806

Refactor avoid to take a con_set, not a con_env

a8f87df

Drop con_env argument to Type.avoid (use kind inside)

0986cff

Remove con_env from scope

51fc8f7

In `typing.ml`, we replace it with a `con_set`, so that we can still call `avoid`. In `check_ir.ml` we simply drop the field.

Actually, get rid of Typing.con_set

933c928

it is always the range of `typ_env` anyways. This simplifies a few more functions.

Delete definition of Type.con_env

0838bc8

nomeata added 3 commits February 15, 2019 10:01

Add call to check_prog back in simploop.ml

f59be98

Make Check_ir dump the IR only upon -v (and tell the user about it)

53d1e78

Fix Construct.labelE (it needs to set the return type)

041c1ce

nomeata added a commit that referenced this pull request Feb 15, 2019

Make check_ir dump upon failure

1fa6712

and other improvements cherry-picked from #171 and #146, namely: * add an interface file (and remove code known to be dead) * make the `Check_ir.env` abstract

nomeata mentioned this pull request Feb 15, 2019

Make check_ir dump upon failure and call Check_ir only from pipeline #176

Merged

crusso reviewed Feb 15, 2019

View reviewed changes

src/construct.ml Show resolved Hide resolved

crusso reviewed Feb 15, 2019

View reviewed changes

src/construct.ml Outdated Show resolved Hide resolved

crusso reviewed Feb 15, 2019

View reviewed changes

src/construct.ml Show resolved Hide resolved

crusso reviewed Feb 15, 2019

View reviewed changes

nomeata changed the base branch from joachim/con-ann to master February 21, 2019 17:57

simploop: move into desugar, use less ANF; Question about array-gen.as

629b881

matthewhammer closed this Feb 26, 2019

nomeata deleted the simploop branch February 27, 2019 11:52

Conversation

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

Uh oh!

Loop transformations

Uh oh!

nomeata commented Feb 5, 2019

Uh oh!

matthewhammer commented Feb 5, 2019

Uh oh!

matthewhammer commented Feb 5, 2019

Uh oh!

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

Uh oh!

Uh oh!

nomeata commented Feb 6, 2019

Uh oh!

matthewhammer commented Feb 6, 2019

Uh oh!

matthewhammer commented Feb 7, 2019

Uh oh!

Uh oh!

crusso Feb 15, 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!

matthewhammer Feb 15, 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!

Uh oh!

Uh oh!

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!

nomeata Feb 15, 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!

crusso Feb 15, 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!

crusso Feb 15, 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!

nomeata commented Feb 25, 2019

Uh oh!

matthewhammer commented Feb 26, 2019

Uh oh!

matthewhammer commented Feb 5, 2019 •

edited

Loading

matthewhammer commented Feb 6, 2019 •

edited

Loading

crusso Feb 15, 2019 •

edited

Loading

matthewhammer Feb 15, 2019 •

edited

Loading

nomeata Feb 15, 2019 •

edited

Loading

crusso Feb 15, 2019 •

edited

Loading

crusso Feb 15, 2019 •

edited

Loading