-
Notifications
You must be signed in to change notification settings - Fork 9
Compatibility with julia nightly #10
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
Conversation
|
All tests (excluding IR equality tests) are passing, so I'm marking this as ready for review! |
Moved to [JuliaLowering](c42f/JuliaLowering.jl#10) instead (the data structure we need is defined there).
Moved to [JuliaLowering](c42f/JuliaLowering.jl#10) instead (the data structure we need is defined there).
|
This is pretty epic, thanks! Doing this port was going to be my first task back to get me back into the swing of things 😆 Regarding the TODOs - definitely best to do them later, except for the tests. We should keep the IR tests working. I'm keen on moving to the use of utility functions like I think we should have |
Sure, will update them
Already done (albeit buried), see aa32c09. I didn't do this at first because I thought both const forms were the same 🙃. Note that this form is probably getting eliminated once JuliaLang/julia#58187 is merged. |
Amazing, thanks! I did see this as soon as I started to do an in depth review. For the tests, you can use the utility function in test/util.jl In addition to existing tests, please add new tests if there's new features to cover. I imagine Can you explain the |
|
Generally, I'll note that I'm taking the occasional liberty with compatibility when I really feel some obscure feature is poorly motivated in modern Julia. Especially when it's an "emergent feature" of the implementation, rather than something done purposefully. There's not many such cases and it's still always a really hard call, but I feel a brand new implementation does provide a little extra freedom here, especially when the code complexity of implementing a mis-feature is high! |
Will do. Once we can use JuliaLowering in core JuliaLang/julia#58207, we can also try using some of the lowering tests from the julia repository without huge manual porting effort.
I'm pretty happy to delete |
|
OK, this is ready for more review. |
This PR along with [the associated JuliaLowering PR](c42f/JuliaLowering.jl#10) allow us to experiment with JuliaLowering as the core lowering implementation, mainly for JETLS development at the moment (cc @aviatesk, @c42f). ## Changes JuliaLowering works with `JuliaLowering.SyntaxTree` input instead of Expr. This change allows`SyntaxTree`s out of parsing and into lowering, hopefully without disturbing existing code expecting Expr. The current design wraps `x::SyntaxTree` like `Expr(:syntaxtree, x)` because otherwise `toplevel_eval_flex` wouldn't know what `x` is, but once we're past the experimental phase, it would only make sense for the compiler to actually know about the types it's operating on. ### New: `Core._lower` It (a julia entrypoint), `jl_lower` (C entrypoint), `jl_fl_lower`, and `fl_lower` all mirror the existing parsing API which was added in #35243. <details> <summary> Click for pre-PR call graph </summary>  </details> After this change:  Like `Core._parse`, `Core._lower` returns an `svec` with the first element as the expected result, and the remaining elements can hold extra information like the lowered SyntaxTree with provenance info (I've left the specification of extra return values for later, when JuliaLowering is more stable and JETLS knows what it needs, because we may opt to pack provenance into the lowered CodeInfo object instead). Parsing only uses the svec to return an offset, but it could actually make use of the the flexibility to return diagnostics in the future. It's unfortunately not clear what types we can expect to go into and come out of lowering (I got Expr, LineNumberNode, String, Symbol, and Nothing in testing.) ### Remove `jl_lower_expr_mod` I realize this isn't being called from C, and Core._lower is a more sensible entry point from julia. We could probably also remove some uncalled parse and jl_load functions. ## Limitations These belong to JuliaLowering, but are listed here as known issues for anyone who wants to try it out. - `(module ...)` forms don't go through JuliaLowering yet. Modules are lowered to a call to `JuliaLowering.eval_module(parentmod, modname, syntaxtree)`, which is currently a stub function that converts to Expr for lowering and evaluation, which JuliaLowering can't use. - Macros work differently. Defining and calling macros should work, but invoking normal macros defined pre-JuliaLowering will not. - Compilation is currently slow (~30 seconds for me).
|
A couple of opaque closure and generated function tests are re-broken on nightly, lol. I will take a look. Edit: Generated function IR tests just need to stay in sync with JuliaSyntax changes (I'm using JuliaLang/JuliaSyntax.jl#552, which hasn't been released). Opaque closures hit assertion failures (unrelated to this change) in |
|
If this gets us closer to master can we merge this and then work on the remaining points after? I'd like to start playing with this so that we can have a concrete plan for 1.13. |
Use the `aviatesk/JuliaLowering.jl#avi/JETLS` branch which combines the following JL branches: - [`mlechu:fix-nightly`](c42f/JuliaLowering.jl#10) - [`aviatesk:avi/precompile`](c42f/JuliaLowering.jl#14) This is a temporary solution until these branches are merged into JL#master. This enables JL precompilation and significantly improves JETLS startup time (even when JETLS `precompile_workload` is set to `false`).
Use the `aviatesk/JuliaLowering.jl#avi/JETLS` branch which combines the following JL branches: - [`mlechu:fix-nightly`](c42f/JuliaLowering.jl#10) - [`aviatesk:avi/precompile`](c42f/JuliaLowering.jl#14) This is a temporary solution until these branches are merged into JL#master. This enables JL precompilation and significantly improves JETLS startup time (even when JETLS `precompile_workload` is set to `false`).
c42f
left a comment
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'm going to try to get through this commit by commit.
First, for CodeInfo changes - looks great though I think we need to set one of the new fields.
(There's also another couple of comments which were sitting around from last time I started going through this - sorry! I think I'll just post them, though)
8b9cd53 to
d180775
Compare
c42f
left a comment
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've tried to review the part of this related to "Updates to const and global lowering" ... I'm not sure I got all the commits related to this, though.
Changes from JuliaLang/julia#57253 (bpart: Fully switch to partitioned semantics). This fixes one failing test and realigns struct desugaring to match lisp for now. Also changed: the expected result of redefining a primitive type (now allowed).
Fix segfaulting test. Thanks for the TODO
Signature changed in JuliaLang/julia#57230. Thanks @aviatesk for the help!
As of JuliaLang/julia#57765, `jl_module_public` is no longer exported. Change our runtime to handle it like `public` and `export` like we handle `import` or `using` for now
I believe this was a world age issue
Too many to count.
Latest julia works. Changes are needed to work with the latest JuliaSyntax, but
that isn't in base julia yet, and more changes are likely to come.
The change lifted the scope of `note`, so it was being changed in the loop
Ping me if you'd like this squashed into the original const/global commit! Co-authored-by: Claire Foster <[email protected]>
No longer needed since we no longer put `global` or `local` forms back into the
expand_forms machine. Some error messages change slightly as a result.
Co-authored-by: Claire Foster <[email protected]>
Thanks! I'm sure there's more we can do here 😊 Ok, so There's admittedly some tension in this - bindings don't obey the scoping rules of the surface language, so the scope resolution pass has no effect on them. From this point of view, it's tempting to use normal
It could be worth revisiting this at some stage. It's not clear whether there would be practical benefits in lowering itself (and there would be some amount of performance cost), though clarifying the role of each pass and separating them cleanly is good in general. |
This makes sense, thanks. My gut tells me it wouldn't be worth changing for the sake of tidiness, but that having bindings not show up in desugaring input could serve as a hint that some other refactor is on the right track. (My dream is to eventually stop feeding so much expand-forms output back into expand-forms.) |
See added comment, and discussion at
c42f#10 (comment)
Co-authored-by: Claire Foster <[email protected]>
test/assignments_ir.jl
Outdated
| 7 (call core.apply_type %₄ %₅ %₆) | ||
| 8 (call core.UnionAll %₃ %₇) | ||
| 9 (= slot₁/X %₈) | ||
| 10 latestworld |
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.
flisp lowering emits this, but I don't think it should.
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.
Seems correct! This is a good sign for the new implementation :-)
84a55b8 to
62a45c6
Compare
Parens are nice, but it wasn't consistent. Also make it a leaf (remaining non-leaves are deleted in the next commit.)
From the docs:
```
The following statements raise the current world age:
1. An explicit invocation of Core.@latestworld
2. The start of every top-level statement
3. The start of every REPL prompt
4. Any type or struct definition
5. Any method definition
6. Any constant declaration
7. Any global variable declaration (but not a global variable assignment)
8. Any using, import, export or public statement
9. Certain other macros like eval (depends on the macro implementation)
```
This commit handles each case as follows:
```
1. = 9
2. I'm not sure this actually happens (or needs to happen, unless we're
being defensive? Doing it after each world-changing operation should
suffice). But if we need it, this would just be emitting once at the
beginning of every lowered output.
3. = 2
4. = 6
5. Emit seeing `method` in linearize
6. Emit seeing `constdecl` in linearize
7. Emit seeing `global` or `globaldecl` in linearize
8. We just defer to `eval`, but should probably go in desugaring later
- using/import recently became builtin calls, and I haven't
updated JL to use them yet. Base._import_using has an expr-based
API that may change, and our importpath destructuring is worth keeping.
- export and public (special forms) are handled in toplevel.c
9. Done for us
```
Other quirks:
- `JuliaLowering.eval_closure_type` calls eval to assign a const, so we still
need to deal with that in closure conversion.
- The `include` hack isn't mentioned in the docs, but can stay in desugaring.
I'm not certain why we don't do the same for non-macro `eval`.
c42f
left a comment
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.
This is looking pretty good now, let's get it merged :)
* Update CodeInfo struct and handling Co-authored-by: Claire Foster <[email protected]> * Don't produce raw symbol from globalref This used to implicitly refer to a module-level name, but lowering is now expected to wrap it in a `globalref`. Part of #54772 * Updates to const and global lowering; add K"constdecl"; omit `wrap` #54773, #56713, #57470. Some changes omitted from `expand-decls` and `expand-assignment`. Note that the two-argument IR "const" is K"constdecl", whereas the one-argument K"const" only appears in the AST. Also note that the `wrap` parameter is omitted throughout assignment desugaring. As far as I'm aware, all this plumbing was just to support `const a,b,c = 1,2,3` having `b` and `c` inherit the `const`. TODO: find a better way of doing the same thing (a ScopedValue might be a clean solution; we currently throw an error). The check for `let; const x = 1; end`, (which should throw) is in scope analysis (lisp has it in `compile`). Co-authored-by: Claire Foster <[email protected]> * Add `isdefinedglobal` builtin #54999, #56985 * :global no longer valid_ir_argument; rm `is_defined_nothrow_global` #56746. Also call :slot and :static_parameter valid (for now) * Fix `is_defined_and_owned_global` (Core.Binding changes) Adapt to bpart changes in #54788 * Struct desugaring: "Undo decision to publish incomplete types..." #56497; Add self-referencing struct shim I have doubts about how long this solution will stay in the base repository, and how complete it is (doesn't seem to work with M1.M2.S), but we are testing for it here. Also change the expected value of a test changed in the same PR. * Emit `latestworld` world age increments For method defs, `latestworld` is produced in desugaring rather than closure conversion for now (our closure conversion doesn't seem to cover the same cases as lisp lowering yet). Covers #56523, #56509, #57299. Also includes changes from #57102 (bpart: Start enforcing minimum world age for const bparts) and #57150 (bpart: Start enforcing min_world for global variable definitions) since the lowering changes from those appear to be amendments to the changes above (missing world age increments). Co-authored-by: Claire Foster <[email protected]> * bpart changes: `Core._typebody!` signature `Core._typebody!` now takes a new "prev" argument, which we don't use yet here. Changes from #57253 * bpart changes: struct desugaring Changes from #57253 (bpart: Fully switch to partitioned semantics). This fixes one failing test and realigns struct desugaring to match lisp for now. Also changed: the expected result of redefining a primitive type (now allowed). * Additional argument in `new_opaque_closure` Fix segfaulting test. Thanks for the TODO * Adapt to different `GeneratedFunctionStub` signature Signature changed in #57230. Thanks @aviatesk for the help! * Fix `public` and `export` As of #57765, `jl_module_public` is no longer exported. Change our runtime to handle it like `public` and `export` like we handle `import` or `using` for now * Fix modules.jl test I believe this was a world age issue * Regenerate IR tests Too many to count. * Update README to known-good julia, JuliaSyntax versions Latest julia works. Changes are needed to work with the latest JuliaSyntax, but that isn't in base julia yet, and more changes are likely to come. * Fix small bug from JuliaLang/JuliaLowering.jl#16 so tests pass The change lifted the scope of `note`, so it was being changed in the loop * Changes from code review: const/global lowering Ping me if you'd like this squashed into the original const/global commit! Co-authored-by: Claire Foster <[email protected]> * Remove a special case No longer needed since we no longer put `global` or `local` forms back into the expand_forms machine. Some error messages change slightly as a result. * Changes from code review Co-authored-by: Claire Foster <[email protected]> * Fix + test for assignment in value but not tail position * Disallow `static_parameter` as `valid_ir_argument` See added comment, and discussion at c42f/JuliaLowering.jl#10 (comment) Co-authored-by: Claire Foster <[email protected]> * Change printing of `K"latestworld"` Parens are nice, but it wasn't consistent. Also make it a leaf (remaining non-leaves are deleted in the next commit.) * Move most `latestworld`s to linearization From the docs: ``` The following statements raise the current world age: 1. An explicit invocation of Core.@latestworld 2. The start of every top-level statement 3. The start of every REPL prompt 4. Any type or struct definition 5. Any method definition 6. Any constant declaration 7. Any global variable declaration (but not a global variable assignment) 8. Any using, import, export or public statement 9. Certain other macros like eval (depends on the macro implementation) ``` This commit handles each case as follows: ``` 1. = 9 2. I'm not sure this actually happens (or needs to happen, unless we're being defensive? Doing it after each world-changing operation should suffice). But if we need it, this would just be emitting once at the beginning of every lowered output. 3. = 2 4. = 6 5. Emit seeing `method` in linearize 6. Emit seeing `constdecl` in linearize 7. Emit seeing `global` or `globaldecl` in linearize 8. We just defer to `eval`, but should probably go in desugaring later - using/import recently became builtin calls, and I haven't updated JL to use them yet. Base._import_using has an expr-based API that may change, and our importpath destructuring is worth keeping. - export and public (special forms) are handled in toplevel.c 9. Done for us ``` Other quirks: - `JuliaLowering.eval_closure_type` calls eval to assign a const, so we still need to deal with that in closure conversion. - The `include` hack isn't mentioned in the docs, but can stay in desugaring. I'm not certain why we don't do the same for non-macro `eval`. --------- Co-authored-by: Claire Foster <[email protected]>
The goal of this change is to have JuliaLowering work on julia nightly again,
particularly for the JETLS development that's going on, by porting recent
changes from julia-syntax.scm. It will be an unfortunately large diff, but a
smaller PR would leave JuliaLowering broken for all versions of julia, so I've
settled on keeping the commit history readable.
Major changes
globalreflatestworld)globaldeclglobalform is still emitted from both lowering implementations, but may not be in the futureconstdeclIR form that takes two arguments like=, not to be confused with the one-argumentconstAST formisdefinedglobalbuiltin::GeneratedFunctionStubandCore._typebody!TODO
For a future PR:
_defaultctors? (internals: add _defaultctor function for defining ctors JuliaLang/julia#57317)Commits considered
This list was generated so I could keep track of my work, but it might be helpful here to list bugfixes we don't have yet. Note that it only includes changes to julia-syntax.scm, which doesn't cover all breaking commits. N/A means I intentionally skipped it for some reason (e.g. already done before this PR).
Long list
outerrefintermediate form (#54772)exportand similar (#54812)Moduleinto method name slot (#54856)@opaque Tuple{T,U...}->RT (...)->...syntax for explicit arg/return types (#54947)PartialOpaquesupport viaExpr(:opaque_closure, ...)(#55068)finallyblocks for exceptional control-flow (#55876):latestworld(#56523)(pop-handler-list ...)(#55871)linearize(#57416)...expressions (#57480)>:inwhere(#57554)functiondeclaration implicitlyglobal(#57562)importorusing(#57774)constassignment:methodExpr return value between interpreter and codegen (#58076)