Continue fixing inlining#992
Open
RenaudFondeur wants to merge 93 commits intopharo-project:pharo-12from
Open
Conversation
…lass each time -> now way faster + easier to debug
…efine node being the same node every where + cleaning
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
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!