Skip to content

Effect Analysis Prototype & Safe Eta Expansion - #562

Open
flandia wants to merge 28 commits into
hkust-taco:hkmc2from
flandia:flow-effect
Open

flandia wants to merge 28 commits into
hkust-taco:hkmc2from
flandia:flow-effect

Conversation

@flandia

@flandia flandia commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

This artifact is endorsed by Anto

@flandia

flandia commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

wtf Codex stole my authorship

Comment thread hkmc2/shared/src/main/scala/hkmc2/codegen/flowAnalysis/FlowAnalysis.scala Outdated
@LPTK

LPTK commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Thanks!

As discussed,

  • let's add a flag to Assign nodes to determine their effectfulness. We should make sure not to treat local mutation as a nonlocal side effect;
  • do not use an effectful annotation (just use actual effects instead);
  • make the optimizer not run when the lifter is disabled, so we don't need to care about nested definitions;
  • merge this into the eta expansion pass (later: the megapass).

Comment thread hkmc2/shared/src/main/scala/hkmc2/codegen/flowAnalysis/FlowAnalysis.scala Outdated
Comment thread hkmc2/shared/src/main/scala/hkmc2/codegen/flowAnalysis/FlowAnalysis.scala Outdated
Comment thread hkmc2/shared/src/main/scala/hkmc2/codegen/CompilationPipeline.scala Outdated
@LPTK
LPTK marked this pull request as draft September 21, 2026 10:37
@flandia
flandia force-pushed the flow-effect branch 2 times, most recently from cafbf36 to 60cb3ae Compare October 2, 2026 03:45
@flandia flandia changed the title Effect Analysis Prototype Effect Analysis Prototype & Eta Expansion Gate Oct 3, 2026
codex and others added 6 commits October 4, 2026 04:06
If lifting doesn't run, then eta expansion doesn't run.

Co-authored-by: Flandia <[email protected]>
Carry annotations with rewritten function parameters and bodies so every definition builder consumes the eta rewrite's metadata. Remove stale @pure annotations when adding stages and mark extended calls as potentially effectful. Preserve existing deforestation annotations.

Co-authored-by: Flandia <[email protected]>
Increment affine annotation indices when lifting adds a capture parameter list, so the annotation continues to describe the original stage. Update the eta-expansion golden output for a captured affine lambda.

Co-authored-by: Flandia <[email protected]>
Comment thread hkmc2/shared/src/main/scala/hkmc2/codegen/EtaExpansion.scala Outdated
Comment thread hkmc2/shared/src/test/mlscript/deforest/eta-expansion/recursive.mls Outdated
@LPTK

LPTK commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Please stop force-pushing to this branch, it makes following the latest changes very hard. Just use merge commits like a civilised person.

@flandia

flandia commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor Author

Please stop force-pushing to this branch, it makes following the latest changes very hard. Just use merge commits like a civilised person.

Oh sorry I wanted to give a clean layer-by-layer review presentation

I'll do that locally next time

@flandia

flandia commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor Author

So I think this is ready for review.

It adds Pure and UnsafeEta summaries to functions and propagates them through calls from flow analysis (hkmc2/shared/src/test/mlscript/effect-analysis/Basic.mls). It conservatively assumes unsafe unless things can be proven pure. If a function is unsafe to eta-expand, the transformation stops at that point but still eta-expands the calls before it (hkmc2/shared/src/test/mlscript/opt/EtaEffects.mls).

Then a set of mutation analysis rules determine which functions are unsafe (hkmc2/shared/src/test/mlscript/effect-analysis/Mutation.mls). Examples with nested functions depend on the lifter so there are some mysterious and unpredictable (but seem to be sound) behaviors... e.g.,

fun f() =
  let state = 0
  fun observe_mutable() =
    state
  set state = 42
  observe_mutable()
  state
//│ flow-analysis > >>> effect-analysis results >>>
//│ flow-analysis > function f#0 @ <root> -> Pure
//│ flow-analysis > function observe_mutable#0 @ <root> -> Pure
//│ flow-analysis > call observe_mutable@result @ <root> -> Pure
//│ flow-analysis > <<< effect-analysis results <<<

fun g =
  let state = 0
  fun observe_mutable() =
    state
  observe_mutable()
  set state = 42
  state
//│ flow-analysis > >>> effect-analysis results >>>
//│ flow-analysis > function g#0 @ <root> -> UnsafeEta
//│ flow-analysis > function observe_mutable#0 @ <root> -> UnsafeEta
//│ flow-analysis > call observe_mutable@result @ <root> -> UnsafeEta
//│ flow-analysis > <<< effect-analysis results <<<

@flandia
flandia marked this pull request as ready for review October 4, 2026 07:24
@flandia

flandia commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Then a set of mutation analysis rules determine which functions are unsafe (hkmc2/shared/src/test/mlscript/effect-analysis/Mutation.mls). Examples with nested functions depend on the lifter so there are some mysterious and unpredictable (but seem to be sound) behaviors... e.g.,

OK I managed to figure this out. It's not mysterious or unpredictable but a bit complex

@flandia

flandia commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

LP has spoken in the meeting; thus we shall change the word Gate

Comment thread hkmc2/shared/src/main/scala/hkmc2/codegen/FlowAnalysisBasedRewrite.scala Outdated
Comment thread hkmc2/shared/src/main/scala/hkmc2/codegen/Lifter.scala Outdated
Comment thread hkmc2/shared/src/main/scala/hkmc2/semantics/Term.scala Outdated
Comment thread hkmc2/shared/src/test/mlscript/codegen/ConfigDirective.mls Outdated

@LPTK LPTK left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The PR tries to add hundreds of lines of unwanted extra output. Please be more careful and review your diff properly. We don't want to stage that much noise.

flandia and others added 9 commits October 16, 2026 00:15
# Conflicts:
#	hkmc2/shared/src/main/scala/hkmc2/codegen/FlowAnalysisBasedRewrite.scala
#	hkmc2/shared/src/main/scala/hkmc2/codegen/deforest/Rewrite.scala
#	hkmc2/shared/src/main/scala/hkmc2/codegen/flowAnalysis/FlowAnalysis.scala
#	hkmc2/shared/src/test/mlscript/deforest/eta-expansion/basic.mls
#	hkmc2/shared/src/test/mlscript/deforest/eta-expansion/recursive.mls
#	hkmc2/shared/src/test/mlscript/opt/EtaExpansion.mls
@flandia flandia changed the title Effect Analysis Prototype & Eta Expansion Gate Effect Analysis Prototype & Safe Eta Expansion Oct 15, 2026
Comment on lines -113 to -114
//│ eta-expansion > flatten_append$append⁰#0 @ <root>: [2, 5]
//│ eta-expansion > flatten$flatten⁰#0 @ <root>: [1, 5]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Regression in eta expansion!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reason is overly conservative field access classification. Just fixed

Comment thread hkmc2/shared/src/test/mlscript/effect-analysis/Basic.mls
@flandia

flandia commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

I feel like we should have a #config_once or something for simpler test cases. Now adding a #config for a specific case changes all the subsequent blocks which is annoying and fragile!

Comment on lines 54 to 72
:expect O
fun p() = X(Y(O))
fun f(x) = if x is
X(y) then y
fun g(y) = if y is
Y(o) then o
g(f(p()))
//│ deforest > >>> fusing >>>
//│ deforest > X⁰(tmp⁰) ->
//│ deforest > match: x³
//│ deforest > fields: x³.a⁰
//│ deforest > Y⁰(O⁰) ->
//│ deforest > match: y⁰
//│ deforest > fields: y⁰.b⁰
//│ deforest > <<< fusing <<<
//│ eta-expansion > >>> eta-expansion targets shapes >>>
//│ eta-expansion > f$f⁰#0 @ <root>: [1, 2]
//│ eta-expansion > p$p⁰#0 @ <root>: [0, 2, 2]
//│ eta-expansion > f$x_X⁰#0 @ <root>: [1, 2, 2]
//│ eta-expansion > p$p⁰#0 @ <root>: [0, 2]
//│ eta-expansion > <<< eta-expansion targets shapes <<<
//│ = O

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just tracking this yet another regression. Codex told me fun f(x) = ... is deforested into

fun f$x_X(X_a)(...consumerVariables) =
  X_a

then X_a becomes a capture, so our analysis classifies it as unsafe. This should be fixed by the capture analysis later

@LPTK

LPTK commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

As discussed, pls ensure that there are no regressions to the existing committed tests – use @pure annotations with FIXMEs if needed. And investigate the problem in #562 (comment) to actually understand it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants