-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Refactor Binding
data structures in preparation for partition
#54788
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
src/codegen.cpp
Outdated
jl_value_t *ty = jl_atomic_load_relaxed(&bnd->ty); | ||
jl_binding_partition_t *bpart = jl_get_binding_partition(bnd, ctx.max_world); | ||
if (bpart && !bpart->constp) { | ||
jl_value_t *ty = bpart->restriction; | ||
if (ty != nullptr) { |
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 believe this needs additional guards now, for this case:
julia> global x::Int64
julia> foo() = (global x; x)
julia> foo() # emits code w/ stores/loads directly to binding->value
julia> const x = 15 # does not update binding->value any more
julia> foo() # should be 15
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.
Invalidation will take care of this.
df97a48
to
dfaa660
Compare
src/julia.h
Outdated
* First look at ->imported (see above). If NONE, but `->constp`, this holds the | ||
* value of the constant. Otherwise this is a global and holds the type restriction. | ||
*/ | ||
jl_value_t *restriction; |
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.
Would it be worth using Core.Const(...)
here, instead of the constp
logic?
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 that would be semantically consistent and reasonable, but there isn't really any benefit to doing so.
// ->restriction does not hold any import-related data. | ||
BINDING_IMPORT_NONE = 0x0, | ||
// Implicit: The binding was implicitly import from a `using`'d module. | ||
// ->restriction holds the imported binding |
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.
Is this still a short-circuit link to the defining module, or is it now the immediate module involved in the local using
/import
?
I'm assuming that we're expecting to maintain backlinks from all the intermediate modules that a binding was resolved through, so that a change to any of them splits all downstream binding partitions?
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.
Yes, it short circuits. I haven't worked out the backedges yet.
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 changed my mind on this. It no longer short circuits in the latest version.
src/julia.h
Outdated
uint8_t constp:1; | ||
uint8_t exportp:1; // `public foo` sets `publicp`, `export foo` sets both `publicp` and `exportp` | ||
uint8_t publicp:1; // exportp without publicp is not allowed. | ||
uint8_t imported:1; | ||
uint8_t imported:2; // enum jl_binding_import | ||
uint8_t usingfailed:1; |
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.
Perhaps this should be BINDING_IMPORT_FAILED
now
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.
Yes
499eed2
to
f00f2e6
Compare
890e668
to
c2cc636
Compare
@nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. |
@nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. |
@nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. |
aa4b302
to
d1d842e
Compare
@nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. |
@nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. |
@nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. |
Alright, I've restored all the old semantics, with the exception of
and other attempts to redefine constants using setglobal!, which is now an error (and will be after full partition). Constant redefinition using the keyword, i.e.
and remains undefined behavior (for now). I'm planning to address the atomics concerns and then merge this in the near future. |
I think this should be good to go now. I've gone through and addressed the review comments. I'll run another round of pkgeval. Unless there's something pressing, I think I'll take anything additional post-commit since this is one of those branches that keeps accumulating conflicts and I need to keep the full partition branch up-to-date as well. |
For compat with current semantics.
@nanosoldier |
@nanosoldier |
This comment was marked as resolved.
This comment was marked as resolved.
The package evaluation job you requested has completed - possible new issues were detected. |
This seems to break CI consistently: #55642 EDIT: also, the PkgEval above seems to find an "unreachable reached" situation in one or two packages |
This codepath is odd. It derives a method name from the slotname metadata and then does an implicit assignment to that slot. Worse, as the comment indicates, the codepath is missing from the interpreter, which will crash if it were to ever encounter such a piece of code. I suspect this pattern is unused - I accidentally broke it badly in (the as of yet unmerged PR) #54788 and neither tests nor pkgeval noticed. So let's try removing it on master. If it turns out something does depend on it, we can go the opposite way and implement it properly in all the places that look at :method.
This is a re-worked extraction of #54654, adjusted to support the new semantics arrived at over the course of that thread. Note that this is the data-structure change only. The semantics in this PR still match current master to the greatest extent possible. The core idea here is to split `Binding` in two: A new `Binding` with minimal data and a `BindingPartition` that holds all data that is world-age partitioned. In the present PR, these are always in 1:1 correspondednce, but after #54654, there will be multiple `BindingPartition`s for every `Binding`. Essentially the `owner` and `ty` fields have been merged into a new `restriction` field of `BindingPartition`, which may also hold the value of a constant (consistent with the final semantics reached in #54654). The non-partitioned binding->value field is now used exclusively for non-constant globals. The disambiguation for how to interpret the `restriction` field happens via flags. `->imported` grew to 2 bits and can now be one of `NONE`/`IMPLICIT`/ `EXPLICIT`/`GUARD`. `GUARD` corresponds to the old `b->owner == NULL` case. `NONE` corresponds to the old `b->owner == b` case, while IMPLICIT/EXPLICIT correspond to `b->owner != b` and the old `imported` flag. Other than that, the behavior of the flags is unchanged. Additionally, fields are provided for `min_world`/`max_world`/`next`, but these are unused in this PR and prepratory only.
Adapt to Core.Binding changes in JuliaLang/julia#54788. Also remove `is_defined_nothrow_global` (JuliaLang/julia#56746)
Adapt to Core.Binding changes in JuliaLang/julia#54788. Also remove `is_defined_nothrow_global` (JuliaLang/julia#56746)
Adapt to Core.Binding changes in JuliaLang/julia#54788. Also remove `is_defined_nothrow_global` (JuliaLang/julia#56746)
Adapt to Core.Binding changes in JuliaLang/julia#54788. Also remove `is_defined_nothrow_global` (JuliaLang/julia#56746)
Adapt to Core.Binding changes in JuliaLang/julia#54788. Also remove `is_defined_nothrow_global` (JuliaLang/julia#56746)
Adapt to bpart changes in JuliaLang/julia#54788
Adapt to bpart changes in JuliaLang/julia#54788
Adapt to bpart changes in JuliaLang/julia#54788
Adapt to bpart changes in JuliaLang/julia#54788
* 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 JuliaLang/julia#54772 * Updates to const and global lowering; add K"constdecl"; omit `wrap` JuliaLang/julia#54773, JuliaLang/julia#56713, JuliaLang/julia#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 JuliaLang/julia#54999, JuliaLang/julia#56985 * :global no longer valid_ir_argument; rm `is_defined_nothrow_global` JuliaLang/julia#56746. Also call :slot and :static_parameter valid (for now) * Fix `is_defined_and_owned_global` (Core.Binding changes) Adapt to bpart changes in JuliaLang/julia#54788 * Struct desugaring: "Undo decision to publish incomplete types..." JuliaLang/julia#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 JuliaLang/julia#56523, JuliaLang/julia#56509, JuliaLang/julia#57299. Also includes changes from JuliaLang/julia#57102 (bpart: Start enforcing minimum world age for const bparts) and JuliaLang/julia#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 JuliaLang/julia#57253 * bpart changes: struct desugaring 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). * Additional argument in `new_opaque_closure` Fix segfaulting test. Thanks for the TODO * Adapt to different `GeneratedFunctionStub` signature Signature changed in JuliaLang/julia#57230. Thanks @aviatesk for the help! * Fix `public` and `export` 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 * 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 #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 #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]>
This is a re-worked extraction of #54654, adjusted to support the new semantics arrived at over the course of that thread. Note that this is the data-structure change only. The semantics in this PR still match current master to the greatest extent possible.
The core idea here is to split
Binding
in two: A newBinding
with minimal data and aBindingPartition
that holds all data that is world-age partitioned. In the present PR, these are always in 1:1 correspondednce, but after #54654, there will be multipleBindingPartition
s for everyBinding
.Essentially the
owner
andty
fields have been merged into a newrestriction
field ofBindingPartition
, which may also hold the value of a constant (consistent with the final semantics reached in #54654). The non-partitioned binding->value field is now used exclusively for non-constant globals. The disambiguation for how to interpret therestriction
field happens via flags.->imported
grew to 2 bits and can now be one ofNONE
/IMPLICIT
/EXPLICIT
/GUARD
.GUARD
corresponds to the oldb->owner == NULL
case.NONE
corresponds to the oldb->owner == b
case, while IMPLICIT/EXPLICIT correspond tob->owner != b
and the oldimported
flag. Other than that, the behavior of the flags is unchanged.Additionally, fields are provided for
min_world
/max_world
/next
, but these are unused in this PR and prepratory only.