Migrate some passes to Mini passes #11191

Akirathan · 2024-09-26T19:32:17Z

Pull Request Description

Gets ready for avoiding IR traversal by introducing mini passes as proposed by #10981:

creates MiniPassFactory (that extends common IRProcessingPass) to transform an IR element to another IR element
modifies PassManager to recognize such mini passes and treat them in a special way - by using MiniIRPass.compile
MiniIRPass.compile is using IR.mapExpressions to traverse the IR - alternative approach withNewChildren rejected for now, see future work for details
unlike mega passes IRMiniPass.compile does not recursively traverse, but with 0964711 it invokes each mini pass at constant stack depth - way better for profiling
MiniIRPass.prepare works on edges since ffd27df - there is IRMiniPass prepare(parent, child) to collect information while pre-order traversing from a particular IR parent to a particular IR child
PassManager rewritten to group subsequent mini passes together by MiniIRPass.combine and really and traverse the IR just once - done in 2736a76
converted to mini pass: LambdaShorthandToLambda, OperatorToFunction, SectionsToBinOp and TailCall
tested for 1:1 compatibility by converting original code to test code and comparing IR produced by old and new implementations

Checklist

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

All code follows the
Scala,
Java,
style guides.
Unit tests have been written where possible.
define the mini passes API
rewrite some of the existing passes to mini passes
test the rewritten mini passes are doing the same thing as IRPasses
replace the rewritten IRPasses by the mini passes
verify test/*_Tests continue to work with mini passes
evaluate (there is no negative) impact on benchmarks

Future Work

Pendings for some subsequent work:

rewrite more mini passes and do it in Java - follow SectionToBinOp example - Rewrite more of _mega passes_ to _mini passes_ #11326
parallelize the traversal (needs non-recursive transformation)
explore withNewChildren approach while [PoC] Generate IR definitions with annotation processor #11267

engine/runtime-compiler/src/main/java/org/enso/compiler/pass/MiniIRPass.java

engine/runtime-compiler/src/main/java/org/enso/compiler/pass/MiniPassFactory.java

engine/runtime-compiler/src/main/java/org/enso/compiler/pass/MiniPassTraverser.java

...ntime-integration-tests/src/test/java/org/enso/compiler/test/mini/passes/MiniPassTester.java

engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/expression/Operator.scala

…lly.

…st cases.

Akirathan · 2024-09-30T18:15:44Z

LambdaShorthandToLambda successfuly migrated to mini pass version, implemented by LambdaShorthandToLambdaMini and tested in LambdaShorthandToLambdaTest.

This is an example of a pass that needs to use MiniIRPass.prepare method because it handles blank arguments of Application.Prefix specifically. As you can see, the mini version of the pass is almost 1-1 copy of the previous version with two differences:

prepare method - that attaches ShouldSkipMeta to some Blank IRs.
The transform methods do not traverse the rest of the tree.

The only hack I needed to introduce is a special handling for withNewChildren for DefinitionArgument.Specified in MiniPassTraverser.copyWithNewChildren. Because when you pass 2 arguments to DefinitionArgument.Specified.withNewChildren, it is not possible to determine whether the second child should be ascribedType or defaultValue.

Please, provide your opinions and thoughts @JaroslavTulach , @hubertp and @4e6.

...ime-compiler/src/main/scala/org/enso/compiler/pass/desugar/LambdaShorthandToLambdaMini.scala

enso-bot · 2024-10-14T06:04:24Z

Jaroslav Tulach reports a new STANDUP for yesterday (2024-10-13):

Progress: .

cleanup passes: 64b013f
rewriting SectionsToBinOp to mini: f54ba6d
MiniIRPass.combine: a4ca680
sharing the compileRecursively code
bisecting failures - it is the MiniIRPass.combine
backing out failed changeset: Migrate some passes to Mini passes #11191 (comment)
MiniIRPass.compile: 8124e1e
replacing recursive descent with a queue It should be finished by 2024-10-16.

hubertp · 2024-10-14T08:26:50Z

There seems to be a problem with a4ca680 - the result of applying the three mini passes at once isn't consistent with applying them one by one. Backing the change out.

Isn't that the whole point of this exercise? Even if we are not gaining (or loosing) much in performance, I would feel more confident with merging this if we could prove that it works on those simple passes.

…ertInlineCompilation

…each mini pass

JaroslavTulach · 2024-10-14T09:27:13Z

There seems to be a problem with a4ca680 - the result of applying the three mini passes at once isn't consistent with applying them one by one. Backing the change out.

Isn't that the whole point of this exercise? Even if we are not gaining (or loosing) much in performance, I would feel more confident with merging this if we could prove that it works on those simple passes.

(Alas) that's correct. Let's try it. Update: Done. Described at this comment.

Also, to set the base benchmark state I did VisualVM profiling for ffd27df and while running

Generating index for built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Base

it is apparent we spent 300ms in org.enso.compiler.pass.MiniIRPass.compile out of 12s of runCompilerPipeline. The time is split into 200ms in typing passes:

and yet another 100ms later in tail call mini pass:

There are tiny numbers in the whole compiler processing. Let's see what they do when we combine the passes. Anyway they also confirm there are unlikely to be any regressions as the mini passes execute quite quickly even right now when executed one by one.

Akirathan

I am a bit hesitant to approve the PR (not only because GH won't let me since I am the original author of the PR). I am not entirely happy about the API in MiniIRPass, but there is nothing better we can do at the moment. I like that MiniPassTraverser is no longer recursive, but works with a job queue. I don't understand the implementation, but I trust it does its job.

What I don't like is the fact that TailCall.Mini does all the work inside the prepare phase, i.e., it attaches all the metadata in prepare method, and does nothing in transform. There are two problems with that. 1) Hard to profile - since you have to remember that for TailCall, you need to profile prepare and not transform. 2) Wrong from the rational perspective. Originally, I even wanted to add an assert to the MiniPassTraverser that the prepare methods of minipasses not only change the IR itself, but don't modify any metadata there.

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/TailCall.scala

enso-bot · 2024-10-15T03:56:56Z

Jaroslav Tulach reports a new STANDUP for yesterday (2024-10-14):

Progress: .

updating summary for review: Migrate some passes to Mini passes #11191
changing to prepare(parent, child)
benchmarking mini pass one by one approach: Migrate some passes to Mini passes #11191 (comment)
trying to run multiple mini passes at once - fails
- the problem is in prepare they have restrictions on ability to be merged
pair debugging with Adam to find -Xss2M to resolve his StackOverflowError issue
standups and (demo) meetings It should be finished by 2024-10-16.

JaroslavTulach · 2024-10-15T04:43:27Z

There seems to be a problem with a4ca680 - the result of applying the three mini passes at once isn't consistent with applying them one by one. Backing the change out.

Isn't that the whole point of this exercise? Even if we are not gaining (or loosing) much in performance, I would feel more confident with merging this if we could prove that it works on those simple passes.

Done in 2736a76, @hubertp. Described at this comment.

JaroslavTulach · 2024-10-15T04:52:49Z

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/PassManager.scala

+ flushMiniPass(intermediateIR)
+ } else {
+ intermediateIR
+ }


The 2736a76 commit changes the PassManager to combine subsequent mini passes unless the already collected passes prevent such combining by declaring newly collected pass to be invalidated by their changes. Imagine there is m.enso file:

import Standard.Base.Runtime.Ref.Ref main = r = Ref.new "foo" r.modify (_+"boo") r.get

then the processing of the m.enso file logs following info:

runPassesOnModule[m@AFTER_IMPORT_RESOLUTION] mega running: MethodDefinitions mini collected: org.enso.compiler.pass.desugar.SectionsToBinOp@784abd3e mini collected: OperatorToFunction mini collected: LambdaShorthandToLambda pass OperatorToFunction forces flush before LambdaShorthandToLambda flushing pending mini pass: org.enso.compiler.pass.desugar.SectionsToBinOp$Mini:org.enso.compiler.pass.desugar.OperatorToFunctionMini flushing pending mini pass: org.enso.compiler.pass.desugar.LambdaShorthandToLambdaMini mega running: ImportSymbolAnalysis ...

e.g. three mini passes are collected in this PassGroup. Two of them are "flushed" together by a single IR traversal. The 3rd one is "flushed" separately before a subsequent mega pass is executed.

The LambdaShorthandToLambda is defined as being invalidated by OperatorToFunction pass as it is known that LambdaShorthandToLambda needs to see result of OperatorToFunction in its prepare method - that's only possible if the whole tree produced by OperatorToFunction is fed into LambdaShorthandToLambda - e.g. these two mini passes cannot be merged.

Imagine there is m.enso file:

import Standard.Base.Runtime.Ref.Ref main = r = Ref.new "foo" r.modify (_+"boo") r.get

then the enso --log-level trace --run m.enso file logs following info (after filtering the relevant parts):

[DEBUG] [2024-10-15T06:17:26.979] [org.enso.compiler.pass.PassManager] runPassesOnModule[m@INITIAL] mega running: ModuleAnnotations mega running: DocumentationComments mega running: Imports mega running: ComplexType mega running: FunctionBinding mega running: GenerateMethodBodies mega running: BindingAnalysis mega running: ModuleNameConflicts [DEBUG] [2024-10-15T06:17:28.103] [org.enso.compiler.pass.PassManager] runPassesOnModule[m@AFTER_IMPORT_RESOLUTION] mega running: MethodDefinitions mini collected: org.enso.compiler.pass.desugar.SectionsToBinOp@784abd3e mini collected: OperatorToFunction mini collected: LambdaShorthandToLambda pass OperatorToFunction forces flush before LambdaShorthandToLambda flushing pending mini pass: org.enso.compiler.pass.desugar.SectionsToBinOp$Mini:org.enso.compiler.pass.desugar.OperatorToFunctionMini flushing pending mini pass: org.enso.compiler.pass.desugar.LambdaShorthandToLambdaMini mega running: ImportSymbolAnalysis mega running: AmbiguousImportsAnalysis mega running: org.enso.compiler.pass.analyse.PrivateModuleAnalysis@6540cf1d mega running: org.enso.compiler.pass.analyse.PrivateConstructorAnalysis@ec8f4b9 mega running: ShadowedPatternFields mega running: UnreachableMatchBranches mega running: NestedPatternMatch mega running: IgnoredBindings mega running: TypeFunctions mega running: TypeSignatures [DEBUG] [2024-10-15T06:17:28.141] [org.enso.compiler.pass.PassManager] runPassesOnModule[m@AFTER_GLOBAL_TYPES] mega running: ExpressionAnnotations mega running: AliasAnalysis mega running: FullyQualifiedNames mega running: GlobalNames mega running: TypeNames mega running: org.enso.compiler.pass.resolve.MethodCalls$@bc042d5 mega running: org.enso.compiler.pass.resolve.FullyAppliedFunctionUses$@5484117b mega running: AliasAnalysis mega running: LambdaConsolidate mega running: AliasAnalysis mega running: SuspendedArguments mega running: OverloadsResolution mega running: AliasAnalysis mega running: DemandAnalysis mega running: AliasAnalysis mini collected: TailCall flushing pending mini pass: org.enso.compiler.pass.analyse.TailCall$Mini mega running: org.enso.compiler.pass.resolve.Patterns$@36c2b646 mega running: org.enso.compiler.pass.analyse.PrivateSymbolsAnalysis@37df14d1 mega running: AliasAnalysis mega running: FramePointerAnalysis mega running: DataflowAnalysis mega running: CachePreferenceAnalysis mega running: GenericAnnotations mega running: UnusedBindings mega running: org.enso.compiler.pass.lint.NoSelfInStatic$@7efb53af

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/TailCall.scala

Akirathan

I have just uploaded minor suggestions, feel free to ignore them and integrate.

...untime-compiler/src/main/java/org/enso/compiler/pass/analyse/PrivateConstructorAnalysis.java

Akirathan · 2024-10-15T09:24:15Z

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/IRPass.scala

-
- /** An identifier for the pass. Useful for keying it in maps. */
- val key: UUID @Identifier = IRPass.genId
+trait IRPass extends IRProcessingPass with ProcessingPass {


Minor: What about renaming to MegaIRPass?

I leave that as an exercise for another contender that is using IDE with better refactoring capabilities ;-) Do it as part of

Rewrite more of _mega passes_ to _mini passes_ #11326

Btw. MiniPassFactory should be MiniIRPass and current MiniIRPass should become something else. For example an MiniIRPass.Transformer...

JaroslavTulach · 2024-10-15T14:27:20Z

feel free to ... integrate.

On behalf of @Akirathan and me I approve this PR.

JaroslavTulach

I am glad I could get my hands dirty with this PR. I feel I better understand motives behind our IR. On behalf of Pavel and me, let's approve this change.

enso-bot · 2024-10-16T06:15:11Z

Jaroslav Tulach reports a new STANDUP for yesterday (2024-10-15):

Progress: .

mini passes got merged
- using "invalidated" passes: 2736a76#r1800454373
- introducing IRProcessingPass: 762045a
- rewritten TailCall.Mini: ef3450c
- addressing review comments: 248c0f4
- TailCall rewritten to Java: befef0c
planning meeting It should be finished by 2024-10-16.

Akirathan added 6 commits September 26, 2024 20:19

Implement skeleton of mini pass framework

c9ef2c3

Rename MiniPassManager to MiniPassChainer

43eafae

Implement default IR.withNewChildren

4a0fd85

Add MiniPassTester and MiniPassTraverser

1bbf991

Add tests for OperatorToFunction mini pass

70f0a1a

Add necessary withNewChildren implementations

808dbd8

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 Sep 26, 2024

Akirathan self-assigned this Sep 26, 2024

enso-bot bot mentioned this pull request Sep 26, 2024

Avoid IR traversal by converting (some of) our Passes to mini passes #10981

Closed