cgen: make .global.s work more according to spec
#536
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zerbina
added
refactor
saem
reviewed
Feb 12, 2023
saem
reviewed
Feb 12, 2023
compiler/sem/transf.nim
Outdated
| of nkWhen: | ||
| # a ``when nimvm`` construct | ||
| # XXX: this logic duplciates what ``mirgen`` already does. Maybe | ||
| # collecting should happen there? Or procedure-level globals should |
There was a problem hiding this comment.
Likely makes sense in sem as a normalization step.
zerbina
force-pushed
the
make-globals-work-according-to-spec
branch
from
cceca8c
to
b67d177
Compare
saem
approved these changes
Feb 13, 2023
|
Merge conflict. |
For the C target, globals defined inside procedures that are resource- like (i.e. have a user-defined or lifted destructor) are now initialized in a module's pre-init procedure, instead of each time control-flow reaches the definition. This mirrors what already happened for non-resource-like globals. In addition, globals defined in `inline` procedures no longer cause linking errors because of duplicated definitions in the generated C code. The globals are extracted before translating the AST to MIR code, making the workaround in `mirgen` (that didn't work anyway) obsolete. In order to work towards unifying the architecture of the back-ends, `vmbackend` now also makes use of the pre-extraction, no longer requiring extra logic in the code-generator. - the destructor injection pass is not run for the initializer expression of globals defined inside procedures - `.global.`s on the JS target still don't work according to specification
zerbina
force-pushed
the
make-globals-work-according-to-spec
branch
from
b67d177
to
b114b8d
Compare
|
bors r+ |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
For the C target, globals defined inside procedures that are resource- like (i.e. have a user-defined or lifted destructor) are now initialized in a module's pre-init procedure, instead of each time control-flow reaches the definition. This mirrors what already happened for non-resource-like globals.
In addition, globals defined in
inlineprocedures no longer cause linking errors because of duplicated definitions in the generated C code.Details
The globals are extracted before translating the AST to MIR code, making the workaround in
mirgen(that didn't work anyway) obsolete. In order to work towards unifying the architecture of the back-ends,vmbackendnow also makes use of the pre-extraction, no longer requiring extra logic in the code-generator.Known Issues
.global.s on the JS target still don't work according to specificationNotes for Reviewers
The PR is meant to unblock #450 with the least amount of work and changes to semantics. I think that the introduced regression (no destructor injection for initializers of procedure-level globals) is acceptable until the implementation gets an overhaul.