-
Notifications
You must be signed in to change notification settings - Fork 79
Continue fixing inlining #992
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
Open
RenaudFondeur
wants to merge
93
commits into
pharo-project:pharo-12
Choose a base branch
from
RenaudFondeur:betterInliningMerge
base: pharo-12
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Continue fixing inlining #992
RenaudFondeur
wants to merge
93
commits into
pharo-project:pharo-12
from
RenaudFondeur:betterInliningMerge
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
…lass each time -> now way faster + easier to debug
…efine node being the same node every where + cleaning
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 Pr follows #987 (comment).
Slang doesn't have support for dependency in complex AST branches.
In particular, the bugs in inlining found in the last PR are linked to it.
Information like being inside a return or an assignment is lost easily.
The phases only do checks on the direct children so a case like : 'a = {exp}' is enough to be missed.
Since inlining is really easy to break, my assumptions is that annotation prevents the bugs.
To address this issue, this PR introduces a visitor which links nodes and their effective dependencies, for example :
For the expression 'a = exp1 ifTrue: [exp2] ifFalse: [exp3]' :
- A check by the visitor indicates that exp2 and exp3 are the values used by an assignment (not the 2 '[]', not the conditional itself or 'exp1'/'a').
- The visitor gives for each child of the assignment a reference to the assignment.
- The same goes for return and other nodes have their dependencies indicated (send, switch, ...).
The Pr presents the visitor and its tests plus tests for various bugs in inlining as well as an integration of it in the inlining process.
The visitor allows for :
- Correct indications of return and assignments.
- Automatically prevents inlining of the non-supported case where a method with multiple returns (translated to jump) is inlined in an expression.
The generated code should be equivalent to the old one since we do not create new elements (just transforming to returns/assignment) nor do we end up preventing inlinings of methods (since annotation already does it).
The PR does fix a bug with returning If being considered wrongly as multiple return statements.
the only (non) noticeable change in the inlining phases is that some elements might get their inlining delayed thanks to the new checks :
- For example, checkMethodIntegrityInFrame: is now inlined in pass 5 instead of 3 because it now waits for the inlining of its parents, frameStackValuesWithFP:withSP:do: and framesInPage:do:, it then becomes safe to inline it.
Before it was inlined right away but still ended up as a statement (and hence why it was not annotated as not inlineable).
Other changes:
The inlining process has been refactored further and has been optimized a bit.
- It is now done in place.
- Now we stop trying to inline a method marked as complete so the last passes are really fast, before the information was not used like this because a method could be marked as complete too early and changes could still occur.
- The visitor slows down the process a bit but the refactoring and some optimizations end up making the process to about the same speed.
The generated C code is also a lot cleaner :
- We have more generated comments.
- The Pr fixes some of the non-necessary returns goTo and label generations .
- Assignments and returns are being pushed down in the AST.
- Less non-necessary '{}'.
Tests for dead code elimination and inlining are also much faster now and easier to debug, we now only add the methods needed for the tests instead of the whole class.
Every test of the CI passes except for one, ReflectionMirrors.Primitives.Tests.MirrorPrimitivesTest.testExecutingPrimitive :
which now works but is an expected failure and i don't know why yet.
[edit] : after testing with the pharo-12 branch , the tests is failling also, it seems it was not introduced by this pr
So actually the CI is all good!