Use Module::members -> Dsymbol::codegen to define symbols. #507
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
This commit fundamentally changes the way symbol emission in LDC works: Previously, whenever a declaration was used in some way, the compiler would check whether it actually needs to be defined in the currently processed module, based only on the symbol itself. This lack of contextual information proved to be a major problem in correctly handling emission of templates (see e.g. ldc-developers#454). Now, the DtoResolve…() family of functions and similar only ever declare the symbols, and definition is handled by doing a single pass over Module::members for the root module. This is the same strategy that DMD uses as well, which should also reduce the maintainance burden down the road (which is important as during the last few releases, there was pretty much always a symbol emission related problem slowing us down). Our old approach might have been a bit better tuned w.r.t. avoiding emission of unneeded template instances, but 2.064 will bring improvements here (DMD: FuncDeclaration::toObjFile). Barring such issues, the change shoud also marginally improve compile times because of declarations no longer being emitted when they are not needed. In the future, we should also consider refactoring the code so that it no longer directly accesses Dsymbol::ir but uses wrapper functions that ensure that the appropriate DtoResolve…() function has been called. GitHub: Fixes ldc-developers#454.
Not sure why this wasn't triggered by the test suite before.
Fixes a std.traits test where there would be a mismatch in const/shared qualifiers on the element type of a dynamic array initializer.
The remaining ones should also be easy to remove with a closer look at the situation. Ideally, we would get rid of all of them at some point and use safe wrapper functions for accessing the IrDsymbol associated with a given declaration (which would emit the declarations on the fly if not already present).
This was also broken before the symbol emission changes; we just accidentally managed to avoid the only occurence in the standard library tests.
The codegen parameter was changed to IRState instead of removing it to set the stage for an eventual eradication of the gIR global.
|
@redstar, @AlexeyProkhin: Sorry about this mess, the only interesting commit is the one with the same title as the pull request. The others just clean up things I came across wile touching the better part of the code base, and a few fixes for bugs that were not previously triggered by the test suite for whatever reason. |
|
I think I'll just merge this once it's green and push out another alpha; there isn't really a way to verify this by reviewing. |
dnadlinger
added a commit
that referenced
this pull request
Oct 13, 2013
Use Module::members -> Dsymbol::codegen to define symbols.
|
At least until we don't get a lot more contributors, we should formally establish a post-commit review policy, as done for LLVM. |
redstar
pushed a commit
that referenced
this pull request
Sep 27, 2014
Added the linux inotify subsystem header.
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.
This commit fundamentally changes the way symbol emission in
LDC works: Previously, whenever a declaration was used in some
way, the compiler would check whether it actually needs to be
defined in the currently processed module, based only on the
symbol itself. This lack of contextual information proved to
be a major problem in correctly handling emission of templates
(see e.g. #454).
Now, the
DtoResolve…()family of functions and similar onlyever declare the symbols, and definition is handled by doing
a single pass over
Module::membersfor the root module. Thisis the same strategy that DMD uses as well, which should
also reduce the maintainance burden down the road (which is
important as during the last few releases, there was pretty
much always a symbol emission related problem slowing us
down).
Our old approach might have been a bit better tuned w.r.t.
avoiding emission of unneeded template instances, but 2.064
will bring improvements here (DMD:
FuncDeclaration::toObjFile).Barring such issues, the change shoud also marginally improve
compile times because of declarations no longer being emitted
when they are not needed.
In the future, we should also consider refactoring the code
so that it no longer directly accesses
Dsymbol::irbut useswrapper functions that ensure that the appropriate
DtoResolve…()function has been called.GitHub: Fixes #454.