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

Nockma compile #2570

Merged
merged 65 commits into from
Jan 17, 2024
Merged

Nockma compile #2570

merged 65 commits into from
Jan 17, 2024

Conversation

janmasrovira
Copy link
Collaborator

@janmasrovira janmasrovira commented Dec 29, 2023

This PR is a snapshot of the current work on the JuvixAsm -> Nockma translation. The compilation of Juvix programs to Nockma now works so we decided to raise this PR now to avoid it getting too large.

Juvix -> Nockma compilation

You can compile a frontend Juvix file to Nockma as follows:

example.juvix

module example;

import Stdlib.Prelude open;

fib : Nat → Nat → Nat → Nat
  | zero x1 _ := x1
  | (suc n) x1 x2 := fib n x2 (x1 + x2);

fibonacci (n : Nat) : Nat := fib n 0 1;

sumList (xs : List Nat) : Nat :=
  for (acc := 0) (x in xs)
    acc + x;

main : Nat := fibonacci 9 + sumList [1; 2; 3; 4];
$ juvix compile -t nockma example.juvix

This will generate a file example.nockma which can be run using the nockma evaluator:

$ juvix dev nockma eval example.nockma

Alternatively you can compile JuvixAsm to Nockma:

$ juvix dev asm compile -t nockma example.jva

Tests

We compile an evaluate the JuvixAsm tests in https://github.com/anoma/juvix/blob/cb3659e08e552ee9ca40860077e39a4070cf3303/test/Nockma/Compile/Asm/Positive.hs

We currently skip some because either:

  1. They are too slow to run in the current evaluator (due to arithmetic operations using the unjetted nock code from the anoma nock stdlib).
  2. They trace data types like lists and booleans which are represented differently by the asm interpreter and the nock interpreter
  3. They operate on raw negative numbers, nock only supports raw natural numbers

Next steps

On top of this PR we will work on improving the evaluator so that we can enable the slow compilation tests.

@lukaszcz
Copy link
Collaborator

lukaszcz commented Jan 12, 2024

I refactored the Nockma pipeline to fit more with how other pipelines are handled, moving the JuvixAsm transformations to the function toNockma in Juvix.Compiler.Asm.Pipeline, and adjusting Juvix.Compiler.Pipeline and other places accordingly.

I also removed the --nockma-debug option, because there already is a --debug option. To generate trace pseudo-Nockma instructions, use --debug or -g.

@paulcadman
Copy link
Collaborator

I refactored the Nockma pipeline to fit more with how other pipelines are handled, moving the JuvixAsm transformations to the function toNockma in Juvix.Compiler.Asm.Pipeline, and adjusting Juvix.Compiler.Pipeline and other places accordingly.

I also removed the --nockma-debug option, because there already is a --debug option. To generate trace pseudo-Nockma instructions, use --debug or -g.

Thanks!

@lukaszcz
Copy link
Collaborator

I think ultimately the --nockma-pretty option should also be removed in favour of having a separate "pseudo-nockma" target which supports readable names, debugging instructions, generally some inessential but convenient sugar over the actual Nock format. This target would then be serialized to actual Nockma, omitting the traces and translating names to numbers:

@@ -26,7 +26,7 @@ parseText = Core.runParser replPath defaultModuleId

runRepl :: forall r. (Members '[Embed IO, App] r) => CoreReplOptions -> Core.InfoTable -> Sem r ()
runRepl opts tab = do
embed (putStr "> ")
putStr "> "
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand what happened here and why is it in this PR? We don't need embed anymore? This PR changes that all over the place - it should be separate PR doing this refactor.

Copy link
Collaborator

@paulcadman paulcadman Jan 16, 2024

Choose a reason for hiding this comment

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

Done - the PR was raised #2582

@@ -27,7 +27,6 @@ runCommand opts = do
Geb.ppOut
opts
(tyMorph ^. Geb.typedMorphismObject)
embed $ putStrLn ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why removing newline here (not only embed)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This change was moved to #2582

@@ -114,25 +114,25 @@ checkTypedMorphism gebMorphism = Repline.dontCrash $ do
Right _ -> printError (error "Checking only works on typed Geb morphisms.")

runReplCommand :: String -> Repl ()
runReplCommand input =
runReplCommand input_ =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Analogously with the input that somehow appeared in Prelude or somewhere in this PR and requires adjusting variable names all over the place. This is an unrelated (?) refactor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This change was moved to #2582

FunctionInfo ->
Sem r FunctionInfo
computeFunctionTempHeight tab fi = do
ps :: [Command] <- recurseFun sig fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be done with the simplified recursor recurseS. You don't need full memory information & validity checking here -- just stack height info. I'll change it.

fromSource fileName input =
case runParser fileName input of
fromSource fileName input' =
case runParser fileName input' of
Copy link
Collaborator

@lukaszcz lukaszcz Jan 12, 2024

Choose a reason for hiding this comment

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

These input name changes really should be in a separate PR, if possible. There's too many of them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This change was moved to #2582

@@ -288,7 +288,7 @@ goTopModule cs m = do
htmlOpts <- ask @HtmlOptions
runReader (htmlOpts {_htmlOptionsKind = HtmlDoc}) $ do
fpath <- moduleDocPath m
Prelude.embed (putStrLn ("Writing " <> pack (toFilePath fpath)))
putStrLn ("Writing " <> pack (toFilePath fpath))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same with removing embed

Copy link
Collaborator

Choose a reason for hiding this comment

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

This change was moved to #2582

input <- getFileContents fileName
mp <- runModuleParser fileName input
input_ <- getFileContents fileName
mp <- runModuleParser fileName input_
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated (?) name changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This change was moved to #2582

@@ -25,16 +25,16 @@ import Text.Megaparsec qualified as P
-- | Note: only new symbols and tags that are not in the InfoTable already will be
-- generated during parsing
runParser :: Path Abs File -> ModuleId -> InfoTable -> Text -> Either MegaparsecError (InfoTable, Maybe Node)
runParser fileName mid tab input =
runParser fileName mid tab input_ =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Name change

Copy link
Collaborator

Choose a reason for hiding this comment

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

This change was moved to #2582

@@ -44,14 +45,15 @@ subTermT = go
subTerm :: (Member (Error NockEvalError) r) => Term a -> Path -> Sem r (Term a)
subTerm term pos = do
case term ^? subTermT pos of
Nothing -> throw InvalidPath
-- Nothing -> throw (InvalidPath "subterm")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done

}

data StackId
= CurrentFunction
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you need to store the current function for? "Current function" is never referenced in JuvixAsm - functions are always referenced by name.

= CurrentFunction
| ValueStack
| TempStack
| AuxStack
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's an AuxStack, what do you store there?

| ValueStack
| TempStack
| AuxStack
| FrameStack
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you save the frames in a frame stack? You should just discard them when calling a function and let Nock VM handle that. In fact, you have to discard them for tail recursion to work without leaking memory.


-- Setup function to call with its arguments
-- given n, we compute [R..R] of length n
if
Copy link
Collaborator

@lukaszcz lukaszcz Jan 12, 2024

Choose a reason for hiding this comment

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

You're emulating a call stack on top of a Nock VM, so it's slow. You should not be handling the call stack explicitly -- Nock VM already does this and you should use it. Saving/restoring stack frames is not necessary.

A call should just change the Nock subject (transferring the arguments, etc) and evaluate the compiled function call with the new subject. The result of the evaluation should just be pushed on top of the value stack in the calling function.

The ret instruction is then just returns the top of the value stack. I perhaps should've made it more clear that in JuvixAsm ret can only appear in tail positions (like tcall).

For example,

push arg1
push arg2
call f;
P;

should be translated to Nock similar to:

[Push arg1*
  [Push arg2*
    [Push
        ["replace subject with new one, with 2 arguments from top of the stack"; fetch & eval f]
        ["remove the 2nd & 3rd value from top of the value stack (the arguments) - that's one indexing and one replace in Nock"; P*]
    ]
  ]
]

where X* is the translation of X, and Push is the nock "push" combinator 8. Note that in the second branch of the final push (for P*) you get the original calling function's subject - that's why you can always avoid explicitly saving/restoring stack frames and let Nock VM handle this.

In fact, almost always the call in JuvixAsm will be like above with the pushes corresponding to the number of arguments. Then we can compile this to slightly better:

[Push
  [Push arg1*
    [Push arg2*
      ["replace subject with new one, with 2 arguments from top of the stack"; fetch & eval f]
    ]
  ]
  P*
]

(we could in fact always recreate the original call tree and avoid removing the arguments after call return).

It also seems merging the stacks as I initially intended (#2559) might lead to a simpler nock code for "replace subject with new one" (but it's a bit more difficult, because you need to keep track of the value/temporary stack heights). Though I'm not completely sure how much simpler.

It matters a lot here whether you generate 1 or 10 instructions for a common operation. This translates to roughly an order of magnitude difference in the running time of generated programs.

EDIT: JuvixAsm is actually a bit too low level. Translating the value stack explicitly was a bad idea of mine and it's unnecessary, because the value stack just represents an applicative structure which can be represented directly in Nock. It's possible, but cumbersome and unnecessary, to recover the applicative structure from JuvixAsm code. We should merge the current version, then refactor the JuvixCore -> JuvixAsm translation into JuvixCore -> JuvixTree -> JuvixAsm, where JuvixTree is like JuvixAsm except that instead of the value stack there is an applicative structure. Then adapt the Asm -> Nock translation to Tree -> Nock.

-- push the constructor tag at the top
push (OpAddress # topOfStack ValueStack ++ constructorPath ConstructorTag)
push (constructorTagToTerm tag)
testEq
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need to push the stack twice to test for equality, just to immediately replace the pushed values with the equality result? We have [= X Y] in nock, so just use [= [@ S] [K TagValue]]. That would make testing equality several times faster (or more, because slicing/replacement might not be so cheap in Nock VM).

We can't write this the way one writes Haskell, hoping that it'll somehow get optimized later. There's nothing that can optimize this -- we're generating the code and it's all common basic operations.

Copy link
Collaborator

@lukaszcz lukaszcz left a comment

Choose a reason for hiding this comment

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

Okay, there are still a few unrelated refactors, but let's merge it

@lukaszcz lukaszcz merged commit 73364f4 into main Jan 17, 2024
4 checks passed
@lukaszcz lukaszcz deleted the nockma-compile branch January 17, 2024 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants