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

Eliminate references to Truffle nodes & co. in the compiler #8172

Merged
merged 5 commits into from
Oct 30, 2023

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Oct 28, 2023

Pull Request Description

Work on #6100 has resulted in additional cleanups & refactorings of the Enso Compiler & CompilerContext abstraction.

Checklist

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

  • All code follows the
    Scala,
    Java,
  • All code has been tested:
    • All tests continue to pass.

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Oct 28, 2023
@JaroslavTulach JaroslavTulach self-assigned this Oct 28, 2023
@@ -34,8 +34,6 @@ import org.enso.compiler.phase.{
ImportResolver
}
import org.enso.editions.LibraryName
import org.enso.interpreter.node.{ExpressionNode => RuntimeExpression}
import org.enso.interpreter.runtime.scope.ModuleScope
Copy link
Member Author

Choose a reason for hiding this comment

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

No more Node and ModuleScope referenced in the compiler.

import org.enso.interpreter.runtime.data.Type;
import org.enso.interpreter.runtime.scope.LocalScope;
import org.enso.interpreter.runtime.scope.ModuleScope;
Copy link
Member Author

Choose a reason for hiding this comment

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

No more scopes and ExpressionNode in the CompilerContext.

@@ -1,7 +1,8 @@
package org.enso.compiler.codegen
package org.enso.interpreter.runtime
Copy link
Member Author

Choose a reason for hiding this comment

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

IrToTruffle moves next to TruffleCompilerContext implementation as it is runtime related - not compile (e.g. IR processing) time related activity.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Oct 30, 2023

With latest changes (8477a78b54) there are only few remaining references to interpreter in the IR compiler Scala package:

searching for interpreter

Chances are that soon (in another PR) we might be able to create runtime/compiler package. The ApplicationSaturation phase seems to be doing some static optimizations that would automatically happen in runtime for free. I'll try to remove it to see effect on benchmarks - see #8181

Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -2228,4 +2275,13 @@ class IrToTruffle(
)
}
}

private def asScope(module: CompilerContext.Module): ModuleScope = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: as suggests that we have some casting involved, get or infer would seem to suggest that we get the scope for the given module.

Copy link
Member Author

Choose a reason for hiding this comment

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

some casting involved

In such case I have chosen the right name. There is casting hidden in fromCompilerModule and it could fail if some non-TruffleCompilerContext.Module is passed in.

@JaroslavTulach JaroslavTulach merged commit a862ea7 into develop Oct 30, 2023
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/NoNodesInCompiler_6100 branch October 30, 2023 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants