Remove SimpleDCE and add compiler testing #1008
Closed
Conversation
This file contains hidden or 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
aykevl
force-pushed
the
refactor-ir-functions-2
branch
2 times, most recently
from
f85ff86 to
96b95f6
Compare
|
This PR now has merge conflicts that need to be resolved. |
aykevl
force-pushed
the
refactor-ir-functions-2
branch
from
96b95f6 to
1c16246
Compare
|
Rebased. This PR should not be merged before the next release, it's too big and invasive and relies on followup changes (not yet made) to shrink compile time again. |
aykevl
force-pushed
the
refactor-ir-functions-2
branch
from
1c16246 to
18df8c6
Compare
This regression has now been fixed by #1040. |
This makes viewing the IR easier because parameters have readable names. This also makes it easier to write compiler tests (still a work in progress), that work in LLVM 9 and LLVM 10, as LLVM 10 started printing value names for unnamed parameters.
This is important because once we move to compiling packages independently, SimpleDCE can't work anymore. Instead we'll have to compile all parts of a package and cache that for later reuse.
aykevl
force-pushed
the
refactor-ir-functions-2
branch
from
18df8c6 to
0531d26
Compare
This commit adds a very small test case. More importantly, it adds a framework for other tests to be added in the future.
The ir package has long lost its original purpose (which was doing some analysis and optimization on the Go SSA directly). There is very little of it left, which is best integrated directly in the compiler package to avoid unnecessary abstraction.
aykevl
force-pushed
the
refactor-ir-functions-2
branch
from
0531d26 to
c9b1f0f
Compare
aykevl
added a commit
that referenced
this pull request
Jan 16, 2021
These types are often known to the compiler already. Moving them into
the compiler is an important step for two related reasons:
* It makes the compiler better testable. Together with
#1008 it will make it
possible to test the compiler without having to load the runtime
package.
* It makes it easier to compile packages independently as the type
information of the runtime package doesn't need to be present.
I had to use hack to get this to work well: internal/task.Task is now an
opaque struct. This is necessary because there is a dependency from
*runtime.channel -> *runtime.channelBlockedList -> *internal/task.Task.
I don't want to include the definition of the internal/task.Task struct
in the compiler directly as that would make changing the internal/task
package a lot harder and the compiler doesn't need to know the layout of
that struct anyway.
|
This PR has been superseded by other (merged) PRs. |
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.
The first commit removes most of the ir package and either moves the code to compiler/symbol.go or removes it altogether. After this, not much remains of the ir package and it should be removed eventually.
The second commit relies on the added flexibility provided by the first commit to add testing to the compiler package. It only contains one very basic test but it provides a framework for future tests to be added in a similar way.
Warning: this PR may have consequences. I found that the image/png package stopped compiling due to a bug in the compiler (it produces invalid IR), probably because a function that previously was removed with SimpleDCE is now included in the build. It also slows down the compiler significantly, I've measured +12% (driver smoke tests) to +53% (tinygo smoke tests) increase in compile time. However, this PR is one of the many changes necessary to allow packages to be compiled independently which should eventually speed up the compiler by compiling packages in parallel and caching already compiled packages.
Replaces #466.