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

Store FramePointer in IR #10729

Merged
merged 37 commits into from
Aug 16, 2024
Merged

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Jul 31, 2024

Fixes #10129

Pull Request Description

Adds a new compiler pass FramePointerAnalysis that attaches [FramePointer]

) metadata to various IR elements.

FramePointer metadata serves as a replacement for usage of AliasMetadata.Occurence in IrToTruffle.

There are still usages of AliasAnalysis metadata from IrToTruffle. Which means that alias metadata is still deserialized in IrToTruffle, and therefore, we will not see any big startup time improvements. To remove all usages of AliasAnalysis metadata in IrToTruffle, we need to think about how to remove all alias metadata references from LocalScope which seems like a big task for a follow-up PR.

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.

@JaroslavTulach JaroslavTulach mentioned this pull request Aug 3, 2024
4 tasks
@JaroslavTulach
Copy link
Member

I don't see changes in IrToTruffle to use these new metadata. Those are essential in verifying the PR changes are correct.

@Akirathan Akirathan force-pushed the wip/akirathan/10129-cache-frame-pointer branch from 6b148dd to db92068 Compare August 13, 2024 15:11
# Conflicts:
#	engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/PassPersistance.java
#	engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/graph/Graph.scala
@JaroslavTulach
Copy link
Member

JaroslavTulach commented Aug 14, 2024

What does it mean?

org.graalvm.polyglot.PolyglotException: java.lang.UnsupportedOperationException: unimplemented, took 0.34 sec
 INFO ide_ci::program::command: sbt ℹ️ [error]     at org.enso.compiler.pass.analyse.types.TypePropagation$CompilerNameResolution.assertLinkEquals(TypePropagation.java:394)
 INFO ide_ci::program::command: sbt ℹ️ [error]     at org.enso.compiler.pass.analyse.types.TypePropagation$CompilerNameResolution.assertLinkEquals(TypePropagation.java:336)
 INFO ide_ci::program::command: sbt ℹ️ [error]     at org.enso.compiler.context.NameResolutionAlgorithm.resolveName(NameResolutionAlgorithm.java:47)

at https:/enso-org/enso/actions/runs/10373180715/job/28717694507?pr=10729

GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

@Akirathan Akirathan force-pushed the wip/akirathan/10129-cache-frame-pointer branch from 6ec1ba6 to ce74f86 Compare August 14, 2024 10:44
@Akirathan Akirathan force-pushed the wip/akirathan/10129-cache-frame-pointer branch from 6cfb42d to af1b7e8 Compare August 14, 2024 16:52
@Akirathan Akirathan marked this pull request as ready for review August 14, 2024 17:12
@Akirathan Akirathan added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Aug 14, 2024
@Akirathan Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label Aug 15, 2024
Test on Windows fails because of different length of new lines.
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.

The changes in IrToTruffle are good. We no longer need AliasAnalysis for computing FramePointer. Let's get this in.

However, as the primary goal was to speed things up, we need a away to create LocalScope without reading the alias graphs. Can we use Persistance.Reference there?

--- engine/runtime-compiler/src/main/scala/org/enso/compiler/context/LocalScope.scala
+++ engine/runtime-compiler/src/main/scala/org/enso/compiler/context/LocalScope.scala
@@ -31,8 +31,8 @@ import scala.jdk.CollectionConverters._
   */
 class LocalScope(
   final val parentScope: Option[LocalScope],
-  final val aliasingGraph: AliasGraph,
-  final val scope: AliasGraph.Scope,
+  final val aliasingGraph: Persistance.Reference[AliasGraph],
+  final val scope: Persistance.Reference[AliasGraph.Scope],
   final val dataflowInfo: DataflowAnalysis.Metadata,
   final val flattenToParent: Boolean                       = false,
   private val parentFrameSlotIdxs: Map[AliasGraph.Id, Int] = Map()

* analysis
* @return the frame pointer for `id`, if it exists
*/
def getFramePointer(id: AliasGraph.Id): Option[FramePointer] = {
Copy link
Member

Choose a reason for hiding this comment

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

We no longer need LocalScope to compute getFramePointer. That's good.

@Akirathan Akirathan merged commit 53e9980 into develop Aug 16, 2024
41 checks passed
@Akirathan Akirathan deleted the wip/akirathan/10129-cache-frame-pointer branch August 16, 2024 12:28
@Akirathan
Copy link
Member Author

Follow-up task created in #10833

mergify bot pushed a commit that referenced this pull request Aug 19, 2024
Continuation of #10729 and a step towards #10833 to actually speed things up by 10% by delaying loading of `AliasAnalysis` data.
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store FramePointer in the IR and cache it
2 participants