Share UCS splits using new constructs#457
Share UCS splits using new constructs#457chengluyu wants to merge 4 commits intohkust-taco:hkmc2from
Conversation
# Conflicts: # hkmc2/shared/src/test/mlscript/block-staging/Functions.mls # hkmc2/shared/src/test/mlscript/codegen/MergeMatchArms.mls # hkmc2/shared/src/test/mlscript/ucs/general/LogicalConnectives.mls # hkmc2/shared/src/test/mlscript/ucs/normalization/Deduplication.mls
LPTK
left a comment
LPTK
left a comment
There was a problem hiding this comment.
Nice changes, thanks!
Please avoid duplicating specialized splits when they're used several times with the same specialization. (Later, we might also try to preserve more sharing between specializations.)
Also, please add tests for these cases (sharing of specialized splits and lack thereof).
| //│ set arg$Some$0$ = s⁰.value⁰; | ||
| //│ match arg$Some$0$ | ||
| //│ 1 => | ||
| //│ begin |
There was a problem hiding this comment.
FIXME why was there a begin left?
| case Split.Cons(head, tail) => Split.Cons(head, tail ++ those) | ||
| case Split.Let(name, term, tail) => Split.Let(name, term, tail ++ those) | ||
| case Split.Else(_) /* impossible */ | Split.End => those) | ||
| case Split.Else(_) /* impossible */ | Split.End => those |
There was a problem hiding this comment.
| case Split.Else(_) /* impossible */ | Split.End => those | |
| case Split.Else(_) => softAssert(false); those | |
| case Split.End => those |
| * placement is deferred to the lowest common ancestor of their UseSplit references. */ | ||
| case class JoinPointCtx(pending: Set[SplitSymbol]): | ||
| def +(sym: SplitSymbol): JoinPointCtx = JoinPointCtx(pending + sym) | ||
| case class JoinPointCtx(pending: Set[SplitSymbol], sharingThreshold: Opt[Int]): |
There was a problem hiding this comment.
Is there a reason to put that field in here, when the Config is already available everywhere?
| //│ > x = 42 | ||
| //│ > x | ||
|
|
||
| :fixme // Label/Break blocks from LetSplit lowering not yet supported in ReflectionInstrumenter |
There was a problem hiding this comment.
Please reintroduce this fixme and the conditions to make it break.
| case _ => lhs | ||
|
|
||
| inline def apply(split: Split): Split = normalize(split)(using VarSet()) | ||
| inline def apply(split: Split): Split = normalize(split)(using VarSet(), JoinPointCtx.empty)._1 |
There was a problem hiding this comment.
Note: bug. Should be fixed by moving out the needless field.
| //│ set scrut = true; | ||
| //│ let scrut, scrut1, tmp, tmp1; | ||
| //│ set scrut = true; | ||
| //│ block σ$x: |
There was a problem hiding this comment.
This label seems unusedd. Is this on purpose?
| //│ if (scrut1 === true) { | ||
| //│ Predef.print(1); | ||
| //│ continue lbl | ||
| //│ σ$x: { |
There was a problem hiding this comment.
Why do we ahve a label here? It seems there is no sharing.
| // > $j | ||
|
|
||
|
|
||
| // * Contemplate this absolutely horrendous code (before optimization): |
2 participants
During UCS normalization, when the same alternative split appears on both the positively (
+) and negatively (-) specialized splits of a match, the old implementation duplicated the code. This PR introduces a way to let-bind splits to share the common splits instead.New
SplitconstructsTwo new
Splitvariants were introduced:LetSplit(sym, tail): declares a named split. The symbol'sbodyholds the shared split.UseSplit(sym): references a previously declared named split.A new symbol class
SplitSymbolholds the shared body and an optionalLabelSymbolfor lowering.Changes to
NormalizationMethod
normalizenormalizeImplreturns(Split, Set[SplitSymbol]): the normalized split plus anySplitSymbols whoseUseSplitreferences survive but whoseLetSplithasn't been created yet. This letsLetSplitbe placed at the lowest common ancestor of allUseSplitreferences.A
LetSplitis created only when:EndorUseSplit), andMethod
specializespecializereturnsOpt[Split](N= unchanged,S(...)= changed), soUseSplitreferences are preserved when specialization doesn't change the split.Changes to lowering
lowerSplittranslatesLetSplitto aLabelblock andUseSplittoBreak. When the continuation isRet/Thrw, the label is used directly to preserve tail-call position. Otherwise, an exit label with a temp variable is used.What's removed?
createLabelsForDuplicatedBranchesand theLabelsclass (old label-based deduplication) are gone!patMatConsequentSharingThresholdconfig option and its:patMatConsequentSharingThresholdtest command are also removed. I'm not sure about this step, we can also introduce them back.