[WIP] move things into different passes #85

alaviss · 2021-05-06T01:25:16Z

This thing is made of hacks, don't use, just look.

scratch.nim is the simple test that compile.

taste won't compile.

saften is for the transformation only

A lot of things doesn't work, one of them is compiling taste. scratch.nim contains a simple test case that does compile. This will be rewritten soon-ish.

Not sure why the compiler produces this stuff, maybe its an artifact of desugaring `return expr` into `result = expr; return`. Except its `return result = expr` in this case. However we cannot rewrite this into `result = expr; return`, because the compiler wouldn't like that if the proc is nested within an another proc that returns (due to closure capture analysis), even if you use the `result` symbol within the return statement (the compiler is broken beyond help).

Sem-ed pragmas that are not built in will be rewritten into calls or a colon expression. This commit add support for detecting and filtering those pragmas.

Instead of making cpsJump an annotation so that other scope macros may direct the flow, we employ the use of a custom pragma as an annotation to where the control-flow continuation should be inserted. This drastically simplifies the work that scope macros will have to do, since all transformations would have been done before they reaches the scope macros.

as discussed with disruptek

This allows the code to be shared between cps transforms macros.

This is a simple primitive that says: Going from `a` to `b` might not be a linear transition because `a` might be tainted by a cpsJump. It doesn't work on the simple sample yet, got some internal errors.

This keeps the sem-ed result of each replacement separated, preventing weird compiler errors.

Got rid of some crashes but now we got varargs issues.

normalizingRewrites is now the universal tool to turn typed AST into something that's safe to consume. This also makes normalizingRewrites recurse into what it rewrote.

These macros modify call nodes, so they have to be rewritten to force sem to re-evaluate the parameters.

We also normalize the call so that it's safe to remove, a bug discovered on more complex tests.

…inuation Per disruptek's suggestions

sem might flatten the AST so that a tree like this: StmtList IfStmt got passed to those macros as: IfStmt We workaround this issue by putting things in nnkStmtList as needed.

disruptek · 2021-05-08T17:23:56Z

Looks great and I hope we don't have to hold this up with getting anything more than tzevv blocks to pass. There's already a newly-working taste test and this is already good work we shouldn't have to review twice. 😁

cps.nim

disruptek · 2021-05-08T17:06:46Z

cps.nim

+      when false:
+        # if the child is a cps block (not a call), then push a tailcall
+        # onto the stack during the saftening of the child
+        if nc.kind notin unexiter and nc.isCpsBlock and not nc.isCpsCall:
+          withGoto env.splitAt(n, i, "done"):
+            result.doc "saftening during done"
+            result.add env.saften(nc)
+            # we've completed the split, so we're done here
+            return
+      else:


Let's just get rid of it.

disruptek · 2021-05-08T17:07:00Z

cps.nim

+      else:
+        # we want to make sure that a pop inside the after body doesn't
+        # return to after itself, so we don't add it to the goto list...
+
+        let after = splitAt(env, n, i, "afterCall")
+        result.add:
+          tailCall(env, env.saften nc):
+            returnTo after
+        # include the definition for the after proc
+        if after.node.kind != nnkSym:
+          # add the proc definition and declaration without the return
+          for child in after.node.items:
+            if child.kind == nnkProcDef:
+              result.add child
+        # done!
+        return


Let's just get rid of it.

disruptek · 2021-05-08T17:11:14Z

cps.nim

+proc replacePending(n, replacement: NimNode): NimNode =
+  ## Replace cpsPending annotations with something else, usually
+  ## a jump to an another location. If `replacement` is nil, remove
+  ## the annotation.
+  proc resolved(n: NimNode): NimNode =
+    case n.kind
+    of nnkPragma:
+      # a {.cpsPending.} annotation is a standalone pragma statement
+      if n.len == 1 and n.hasPragma("cpsPending"):
+        if replacement.isNil:
+          result = newEmptyNode()
+        else:
+          result = copyNimTree replacement
+    else: discard
+
+  result = filter(n, resolved)


Prefer an additional version that takes no replacement argument and does the right thing, versus a version that has special logic to handle a sentinel nil. The current version should demand that the replacement is not nil.

disruptek · 2021-05-08T17:13:01Z

cps.nim

+  # make `n` safe for modification
+  let n = normalizingRewrites n
+  # replace all `pending` with `return nil`, signifying end of continuations
+  result = replacePending(n, tailCall(nil, nil))


Prefer not providing replacePending a second argument, but failing that...
Prefer a tailCall that does the right thing with no input... same as my prior feedback; this is very smelly. 😁

disruptek · 2021-05-08T17:13:58Z

cps.nim

+
+  debug(".cpsStripPending.", result, akOriginal, n)
+
+proc xfrmFloat(n: NimNode): NimNode =


Maybe another rewrite name like rewriteLift is more obvious?

Lift brings it into the CPS context, more precise than rewrite being flatten?

tests/scratch.nim

disruptek · 2021-05-08T17:18:43Z

cps/spec.nim

+    let
+      info =
+        if info.isNil:
+          n
+        else:
+          info
+      lineInfo = info.lineInfoObj
+      procName =
+        if info.kind in RoutineNodes:
+          repr info.name
+        else:
+          ""


Feel free to make this 3-4 lines.

There is no harm in wrapping multiple StmtList Co-authored-by: Smooth Operator <disruptek@users.noreply.github.com>

The new branch is starting to work well now.

This is no longer necessary now that taste compiles

It's no longer saften business

Named block, simple block, all supported.

differences with previous: - all block statements are rewritten if they contain a cpsJump, regardless of their position in the AST. This is required due to interactions with {.cpsBreak.} - all {.cpsBreak.}, {.cpsContinue.} are now cleaned up on-site if the while/block is not a cps block.

We happened to fix a bug in the process.

The "end of continuation" has been moved into a dedicated proc.

AstKind is now a {.pure.} enum, and we fixes some erroneous annotations

This is a huge simplification of rewrite that we can look to exploit in the future. Annotations creation are also moved to spec.

saem · 2021-05-10T15:37:40Z

cps.nim

-        else:
-          # {.cpsBreak.}
-          tail
+func matchCpsBreak(label: NimNode): Matcher =


matcherCpsBreak, since it creates one rather than matches it? Not a blocker, cpsBreakMatcher reads better but the lack of symmetry sucks for grouping related things. I can do this later too.

disruptek · 2021-05-10T16:02:45Z

Really excellent work, @alaviss! 😘

alaviss added 18 commits May 3, 2021 20:13

cps: move nnkConv rewrite into normalizingRewrites

8b618ae

saften is for the transformation only

[WIP] cps: move jumping after cpsCall into a separate stage

7bad077

A lot of things doesn't work, one of them is compiling taste. scratch.nim contains a simple test case that does compile. This will be rewritten soon-ish.

cps/spec: supports scanning for desugared pragmas

ded4987

Sem-ed pragmas that are not built in will be rewritten into calls or a colon expression. This commit add support for detecting and filtering those pragmas.

cps: some syntax fixes

b9e8c78

cps: some cleanup

c1824a1

as discussed with disruptek

cps: move debugging code to a func

2f2a51e

This allows the code to be shared between cps transforms macros.

cps: introduce cpsMayJump

3fdf8f4

This is a simple primitive that says: Going from `a` to `b` might not be a linear transition because `a` might be tainted by a cpsJump. It doesn't work on the simple sample yet, got some internal errors.

cps: replacePending now copy the replacement

9ddcdae

This keeps the sem-ed result of each replacement separated, preventing weird compiler errors.

cps: fix cpsJump debug msg

f8c9932

cps: run more de-sem stuff on cpsJump ast

9d0d5ee

Got rid of some crashes but now we got varargs issues.

cps: properly run unhide on the continuation body

270579b

cps: merge normalizingRewrites and unhide

b17890f

normalizingRewrites is now the universal tool to turn typed AST into something that's safe to consume. This also makes normalizingRewrites recurse into what it rewrote.

cps: run workaroundRewrites on cpsJump and cpsMayJump

210bb53

These macros modify call nodes, so they have to be rewritten to force sem to re-evaluate the parameters.

cps: support jump to nothing

95cd415

We also normalize the call so that it's safe to remove, a bug discovered on more complex tests.

cps: use firstReturn to figure out whether there is control flow cont…

3fa2897

…inuation Per disruptek's suggestions

cps: add support for non nnkStmtList to cpsJump and cpsMayJump

bb709c5

sem might flatten the AST so that a tree like this: StmtList IfStmt got passed to those macros as: IfStmt We workaround this issue by putting things in nnkStmtList as needed.

disruptek requested changes May 8, 2021

View reviewed changes

alaviss and others added 10 commits May 8, 2021 13:59

cps: make new StmtList node unconditionally

5b73b7b

There is no harm in wrapping multiple StmtList Co-authored-by: Smooth Operator <disruptek@users.noreply.github.com>

cps: syntax fixes

ce127b7

cps: get rid of old saften code

0f03b57

The new branch is starting to work well now.

scratch.nim: remove

a4c556c

This is no longer necessary now that taste compiles

cps: block and while statements can be tainted too

992fa45

cps: disable simple return optimization in saften

2648268

It's no longer saften business

cps: move block-statement rewrite to a macro

d938e1b

Named block, simple block, all supported.

taste: enable tests that are passing

272132d

cps: move workaroundRewrites to spec

0d814ba

alaviss added 4 commits May 10, 2021 09:53

cps: move annotation detection logic to spec and add replace()

384d060

We happened to fix a bug in the process.

cps: enforce not nil on cont and to of tailCall

8bf029d

The "end of continuation" has been moved into a dedicated proc.

cps: small cleanup on debug

0dfe4d9

AstKind is now a {.pure.} enum, and we fixes some erroneous annotations

cps: clean tailCall a bit

e11e26b

This was linked to issues May 10, 2021

Block rewriting error when there are no explicit break #76

Closed

Splitting inside an if-else generates broken ast #78

Closed

cps: cpsBlock/cpsWhile now compose with cpsMayJump

db0f7d0

This is a huge simplification of rewrite that we can look to exploit in the future. Annotations creation are also moved to spec.

saem reviewed May 10, 2021

View reviewed changes

disruptek marked this pull request as ready for review May 10, 2021 16:00

disruptek merged commit 9c40f1f into nim-works:master May 10, 2021

alaviss deleted the wip-passes branch May 11, 2021 02:58

alaviss mentioned this pull request May 15, 2021

cps: keep going if there is somewhere to go #77

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] move things into different passes #85

[WIP] move things into different passes #85

alaviss commented May 6, 2021 •

edited

Loading

disruptek commented May 8, 2021

disruptek May 8, 2021

disruptek May 8, 2021

disruptek May 8, 2021

disruptek May 8, 2021

disruptek May 8, 2021

saem May 10, 2021

disruptek May 8, 2021

saem May 10, 2021

disruptek commented May 10, 2021


		debug(".cpsStripPending.", result, akOriginal, n)

		proc xfrmFloat(n: NimNode): NimNode =

[WIP] move things into different passes #85

[WIP] move things into different passes #85

Conversation

alaviss commented May 6, 2021 • edited Loading

disruptek commented May 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

disruptek commented May 10, 2021

alaviss commented May 6, 2021 •

edited

Loading