Skip to content

Conversation

RenaudFondeur
Copy link
Collaborator

@RenaudFondeur RenaudFondeur commented Jun 30, 2025

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 :

Screenshot 2025-07-31 at 15 54 22

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!

@RenaudFondeur RenaudFondeur marked this pull request as ready for review July 31, 2025 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant