Preliminary deforestation implementation for hkmc2 #289
Conversation
There was a problem hiding this comment.
It looks like you introduced a lot of duplication in this file, which already contained some duplicated logic (there were two instances of should always ends with "undefined" (TODO: check) – now there are four!!).
Every duplicated implementation is more maintenance burden on future developers. Please be more careful about that and try to share and ruse the logic. Even better if you can leave the place in a better state than you found it, for example by removing the original duplication.
There was a problem hiding this comment.
Thanks for the suggestion! In the new commits I tried to deduplicate the logic in this file. As for the check should always ends with "undefined" (TODO: check), there is now only one place that this needs to be done, but it seems that currently it's not valid that all programs end with "undefined" (some others end with "Unit { ... }")
| @@ -406,6 +406,15 @@ enum Case: | |||
|
|
|||
| sealed trait TrivialResult extends Result | |||
|
|
|||
| type ResultId = Uid[Result] | |||
| object ResultUidHandler extends Uid.Handler[Result] | |||
| object ResultUid extends ResultUidHandler.State: | |||
There was a problem hiding this comment.
The fact that this piece of state is global is a bit concerning... If the actual uid value does not influence the result and is only used to distinguish things, you might as well use the object's identity and avoid adding a field to Result, no?
There was a problem hiding this comment.
Thanks for the suggestion! Indeed the actual uid value does not influence the result and is only used to distinguish things. I have pushed the update to remove the global state, and now this uid is just System.identityHashCode.
There was a problem hiding this comment.
Ok, cool. As long as the result is still deterministic and does not depend on the order of iteration of the hashed data structures, it should be fine. (It's a subtle "if", tho – please check.)
There was a problem hiding this comment.
Indeed I found non deterministic behaviors caused by this, and I have explained them here.
| type CtorToDtor = Map[CtorExpr, CtorDest] | ||
| type DtorToCtor = Map[DtorExpr, DtorSource] | ||
|
|
||
| def removeCtor(ctorDests: CtorToDtor, dtorSources: DtorToCtor, rm: Set[CtorExpr]): CtorToDtor -> DtorToCtor = |
There was a problem hiding this comment.
This code looks extremely inefficient, rebuilding tons of intermediate data structures. As the author of Lumberhack, you should have a clue that this isn't great... 😛
Couldn't you just keep track of ctors and dtors by using a couple of mutable fields in the corresponding ctors and dtors?
There was a problem hiding this comment.
Thanks for the suggestion! I am looking into optimizing this part.
There was a problem hiding this comment.
I have updated this part so that the functions removeCtor and removeDtor directly remove entries in the maps instead of allocating new intermediate maps.
…ck the fusion when this happens
| object StratVarUidHandler extends Uid.Handler[StratVar] | ||
| object StratVarState: | ||
|
|
||
| def freshVar(nme: String = "")(using vuid: Uid.Handler[StratVar]#State) = | ||
| def freshVar(nme: String = "")(using vuid: StratVarUidHandler.State) = |
There was a problem hiding this comment.
Why do you remove all empty lines around these definitions, but use empty lines lower, down?
There was a problem hiding this comment.
Sorry I am not sure if I fully get your question. I think I just removed this empty line (line 29) without thinking about the reason...😨 and by "but use empty lines lower, down" do you mean empty lines at line 35-36? For those I also just randomly kept them there...
There was a problem hiding this comment.
I'm not sure exactly what I meant by "lower, down" either, but your use of whitespace looks weird and inconsistent, mainly around this new StratVarUidHandler definition, which comes right in between the StratVarState class and its companion and is somehow not separated by whitespace below it. By the way, why not renamr StratVarUidHandler to StratVar and move it next to the homonym type definition?
There was a problem hiding this comment.
Thanks I see. I did the renaming, and I also realize that all the handler objects seem to be defined in Uid.scala, so I move the definition there.
…tores identityHashCode
| if rm.isEmpty then () | ||
| else |
There was a problem hiding this comment.
| if rm.isEmpty then () | |
| else | |
| if rm.nonEmpty then |
| val toDeleteDtors = rm.flatMap(r => ctorToDtor.remove(r)).flatMap: | ||
| case CtorDest(mat, sels, _) => mat.keySet.map(s => DtorExpr.Match(s)) ++ sels.map(s => DtorExpr.Sel(s.expr)) | ||
| removeDtor(toDeleteDtors) |
There was a problem hiding this comment.
Still... why do you need flat-mapping over concatenated collections just to effectfully remove the corresponding elements? Why isn't this just using nested foreachcalls? Do you really like useless intermediate collections that much?
There was a problem hiding this comment.
Thanks! I think now I understand. I have pushed the update to remove intermediate collections and directly remove each element.
No description provided.