Fix free variable computation for default clauses in Machine #769
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jiribenes
reviewed
Jan 10, 2025
jiribenes
reviewed
Jan 10, 2025
|
On my machine, this completely unblocks #711 and even on CI, it now works with Unfortunately, it seems to break The crash is here in opt LLVM, I've marked
|
|
Feel free to adopt this PR, I'm not planning to work on it further for now. |
jiribenes
changed the title
phischu
force-pushed
the
fix/more_leaks
branch
from
912de0b to
cdea340
Compare
|
Variables appearing free in normal as well as default clauses were erased multiple times because we used a list instead of a set for tracking the free variables. I've pushed a fix, let me know if this is a valid solution. This fixes the issue with |
|
I've just noticed the force push removed the repro (sigh) and the uncommenting of |
jiribenes
reviewed
Jan 11, 2025
effekt/shared/src/main/scala/effekt/generator/llvm/Transformer.scala
Outdated
Show resolved
Hide resolved
|
I'm a bit lost at fixing def foo(l: List[Int]): Int =
l match {
case Nil() => 42
case _ => foo(l.drop(1))
}
record Bar[T](drinks: List[T])
def bar(bar: Bar[Int]): Int = bar match {
case Bar(_) => foo(bar.drinks)
}
def main() = println(Bar([1, 2, 3]).bar) |
|
Ah, we have to erase the scrutinee in the default clause! |
|
Ooh yes, that's much simpler than what I suspected. How should we differentiate between default and normal clause generation? We use |
FWIW when trying this out on Friday, @phischu tried to intuitively do something like: + def labelClause(clause: machine.Clause, isDefault: Boolean): String = {
- def labelClause(clause: machine.Clause): String = {
implicit val BC = BlockContext()
BC.stack = stack
+ if (isDefault) { ... } else { ... }
...
val defaultLabel = default match {
+ case Some(clause) => labelClause(clause, isDefault = true)
- case Some(clause) => labelClause(clause)
case None =>
val label = freshName("label");
emit(BasicBlock(label, List(), RetVoid()))
label
}
val labels = clauses.map {
+ case (tag, clause) => (tag, labelClause(clause, isDefault = false))
- case (tag, clause) => (tag, labelClause(clause))
} |
marvinborner
force-pushed
the
fix/more_leaks
branch
from
af2e1fd to
2c248b5
Compare
|
If anybody reading this has the time, I'd love to know if the changes from the PR still fix the leak in #711 :) |
|
Yep, just tried and it looks like it's not leaking anymore! |
Great, thanks for confirming! 🎉 |
jiribenes
approved these changes
Jan 14, 2025
EveEme
pushed a commit
to EveEme/effekt
that referenced
this pull request
Jan 20, 2025
…lang#769) Fixes 2 leaks from effekt-lang#758 (`modifyat`, `updateat`) and the leak blocking effekt-lang#711 --------- Co-authored-by: Marvin Borner <git@marvinborner.de> Co-authored-by: Jiří Beneš <mail@jiribenes.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes 2 leaks from #758 (
modifyat,updateat) and the leak blocking #711