Skip to content
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

More IR mini passes #11501

Merged
merged 67 commits into from
Nov 28, 2024
Merged

More IR mini passes #11501

merged 67 commits into from
Nov 28, 2024

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Nov 6, 2024

Closes #11326

Pull Request Description

Mega passes converted to minipasses in Passes.globalTypingPasses group:

val globalTypingPasses = new PassGroup(
List(
MethodDefinitions.INSTANCE,
SectionsToBinOp.INSTANCE,
OperatorToFunction,
LambdaShorthandToLambda,
ImportSymbolAnalysis.INSTANCE,
AmbiguousImportsAnalysis.INSTANCE
) ++ (if (config.privateCheckEnabled) {
List(
PrivateModuleAnalysis.INSTANCE,
PrivateConstructorAnalysis.INSTANCE
)
} else List())
++ List(
ShadowedPatternFields,
UnreachableMatchBranches,
NestedPatternMatch,
IgnoredBindings,
TypeFunctions,
TypeSignatures
)
)

Are:

All of these mini passes are ordered after each other. So now, in Passes.globalTypingPasses group, there are 8 consecutive minipasses.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • Benchmarks are solid:
    • engine benchmarks run #1
      • In run 1, MethodDefinitions is still mega pass. Scheduling another one:
    • engine benchmarks run #2

@Akirathan Akirathan added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Nov 6, 2024
@Akirathan Akirathan self-assigned this Nov 6, 2024
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather not increase the amount of projects that depend on Scala.

build.sbt Outdated Show resolved Hide resolved
});

java.util.List<Definition> withStaticAliases = new ArrayList<>();
for (var def : CollectionConverters.asJava(newDefs)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the above .map body and the for loop body could easily be merged together into one big for loop.

* @param method Non-static method from which a static alias method is generated.
* @return Static alias method for the given {@code method} or null.
*/
private Method.Explicit generateStaticAliasMethod(Method.Explicit method) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR.

What would it take to register only static methods in each type? What's an invocation of (x:Integer).to_text anyway? It should be convertible to Integer.to_text x - e.g. an instance invocation just resolves the right method based on the type of the instances, but then it invokes the static Function.

Benefit:

  • reduce the amount of Function instances in the system by half

Prior art:

  • Graal compiler sees all JVM methods as static (instances methods of a class being static and having this as first argument)
  • the compiler only has special InvokeNode that deals with finding the right method to invoke when virtual dispatch is necessary

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created an issue #11686 from your comment.

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for moving the shared Scala utilities to scala-libs-wrapper and keeping engine-common as lightweight as it was. That was the biggest architectural flaw.

From a code point of view, I believe there is a mistake in ChainedMiniPass - it can never unregister completely from the traversal.

Otherwise it all looks good and thanks for increasing the test coverage.

@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Nov 28, 2024
@mergify mergify bot merged commit b5f1106 into develop Nov 28, 2024
44 checks passed
@mergify mergify bot deleted the wip/akirathan/11326-more-mini-passes branch November 28, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite more of _mega passes_ to _mini passes_
2 participants