-
Notifications
You must be signed in to change notification settings - Fork 53
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
Support compilation to Anoma compatible functions #2652
Conversation
c11d971
to
92a86e4
Compare
92a86e4
to
8f7a642
Compare
@@ -69,7 +69,10 @@ toVampIRTransformations = [CombineInfoTables, FilterUnreachable, CheckVampIR, Le | |||
|
|||
toStrippedTransformations :: [TransformationId] | |||
toStrippedTransformations = | |||
[CombineInfoTables, FilterUnreachable, CheckExec, LambdaLetRecLifting, TopEtaExpand, OptPhaseExec, MoveApps, RemoveTypeArgs] | |||
[CombineInfoTables, FilterUnreachable, LambdaLetRecLifting, TopEtaExpand, OptPhaseExec, MoveApps, RemoveTypeArgs] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order was significant here -- "illegal" things (for CheckExec
- axioms) may get filtered out or brought in from imported modules. Plus, there should be similar checks for Anoma/Nockma and later Cairo. I'll make a commit to this PR to refactor the transformation pipeline to allow for different kinds of checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - we moved the CheckExec to a toExec transformation (
juvix/src/Juvix/Compiler/Pipeline.hs
Line 178 in 8f7a642
coreToMiniC = Core.toExec >=> coreToAsm >=> asmToMiniC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commited a refactor.
return Test {..} | ||
|
||
withRootCopy :: (Prelude.Path Abs Dir -> IO a) -> IO a | ||
withRootCopy action = withSystemTempDir "test" $ \tmpRootDir -> do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stored standard library is not shared between different tests, which defeats per-module compilation and makes the tests run around 4 times slower than they otherwise would.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running the tests concurrently in the same directory triggers the leaking IO bypassing the TaggedLock that we've seen before. It's possible that using Effectful will fix this. An alternative would be to synchronously build the standard library somehow before running the test suite and copy that to each temporary location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow, it works for the Juvix->Native/Wasm compilation tests (in tests/Compilation/positive
). What's different?
I remember I've been dealing specifically with this issue when implementing per-module-compilation, and ultimately used file-level locking which solved the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember what's happening exactly for the main exec pipeline compilation tests, but the stored standard library modules seem to be reused judging by decrease in runtime after first test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The anoma compilation tests are using the same functions to run the pipeline as the main exec compilation tests, yet they fail for the leaking IO / tagged lock issue. TaggedLock in general is too unreliable and I'm thinking about a plan to remove it. I propose to leave the anoma compilation tests as they are (using separate directories) for now until we work out how we will replace tagged lock.
|
||
terminating | ||
map'' {A B} {{Partial}} (f : A → B) (x : List A) : List B := | ||
if (null x) nil (f (phead x) :: map'' f (tail x)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiling this to Nockma relies on Core-level optimizations. The function phead
can internally call a fail
function which takes a string as argument. If Partial
is not specialised and this internal call to fail
is not inlined to directly call the builtin, you'll be translating strings and get a crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd like to keep support for Strings so we can intercept messages from builtin fail: #2665
We've re-enabled support for PrimString in the Anoma target and refactored the failing tests to use failwith
(i.e builtin fail) directly instead of via the Partial trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't completely understand the rationale behind this replacement -- if you have strings in the Nockma backend you can use Partial
, if you don't you can't use failwith
either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I misunderstood something. Our rationale is: we don't support Strings in the Nockma backend yet, but we can support builtin fail because it's compiled directly to OpFail
in Tree and we can ignore the String argument in the compilation:
Tree.OpFail -> return crash |
The proposal in #2665 is to intercept the Strings from OpFail and add them to the CellInfo. This String can be used in the Juvix/Nockma evaluator in the error message of the crash but is not serialised when we output Nockma code for consumption by Anoma/Nockma evaluator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we can support builtin fail because it's compiled directly to
OpFail
in Tree
I think that's not an entirely correct assumption, because of eta-expansions in the pipeline or how failwith
is translated from Internal
to Core
. So lambda-abstractions with string argument type might be added on top of the builtin, and the string argument would be applied to a lambda-function - not to the builtin directly. Otherwise my checks for strings in checkBuiltins'
wouldn't fail with only failwith
because OpFail
is already handled specially there. It might work because of subsequent optimisation which inlines the eta-expanded functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I'll restore the PrimString check and modify the tests to avoid using Partial/failwith.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've now modified the tests to avoid using Partial/failwith and restored the Strings check in the anoma backend.
I added checks for strings in the Anoma pipeline, which makes the 4 tests indirectly depending on them fail. They compiled before because you had Core-level optimizations turned on, which inlined the functions taking strings, but we shouldn't rely on this. |
Should these tests be removed then? |
I don't know, maybe they can be fixed, e.g., by removing functions depending on |
bd5cf23
to
1904c0f
Compare
1904c0f
to
a3d888d
Compare
7df03be
to
ac79511
Compare
src/Juvix/Compiler/Pipeline.hs
Outdated
upToAnoma :: | ||
(Members '[HighlightBuilder, Reader Parser.ParserResult, Reader EntryPoint, Reader Store.ModuleTable, Files, NameIdGen, Error JuvixError, GitClone, PathResolver] r) => | ||
Sem r (Nockma.Term Natural) | ||
upToAnoma = upToStoredCore >>= \Core.CoreResult {..} -> Nockma.TermCell <$> coreToAnoma _coreResultModule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be storedCoreToAnoma
(using storedCoreToTree
) instead of coreToAnoma
. Otherwise, we're doing the Core.toStored
transformations twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change src/Juvix/Compiler/Pipeline.hs as discussed in the comment.
This reverts commit 2a22b2f.
c6ed50f
to
f615a0a
Compare
This PR adds a
anoma
target to thejuvix compile
. This target compiles a Juvixmain
function to a Nockma/Anoma "function". Unlike the native, wasm, and nockma targets the main function may have any type signature.Anoma calling convention
Anoma calls functions by evaluating the formula
[call L replace [RL args] @ S]
against a subject equal to the function. Hereargs
is a Nockma term that evaluates to a tuple of arguments that should be passed to the function.The anoma target compiles the
main
function to Nockma in the same way as the nockma target. The main function is then wrapped to make it compatible with the Anoma calling convention.Testing
The anoma calling convention is unit tested and smoke tested.
This PR also adds versions of the end-to-end compilation tests. Most tests are included, tests for builtin IO operations and string builtins are omitted. Other tests that use Strings have been adapted to use other types that are compatible with this backend.
Nockma REPL
To facilitate testing the Nockma REPL can now load a nockma file as an initial subject.