-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[compiler] rewrite FreeVariables to not duplicate binding structure #14451
Conversation
e1ba041
to
16c1284
Compare
env.getFreeVars | ||
} | ||
|
||
assert(old == new_, s"old: $old\nnew: ${new_}\nir:\n${Pretty.sexprStyle(ir, allowUnboundRefs = true)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that all tests passed with this assertion, checking that the new free variables implementation always agrees with the old.
val combEval = Env.fromSeq(IndexedSeq((accumName, {}), (otherAccumName, {}))) | ||
val combOpFreeVarsCompute = compute(combOp, baseEnv.copy(eval = combEval)) | ||
val combOpFreeVars = combOpFreeVarsCompute.copy( | ||
eval = Env.empty[Unit], | ||
scan = Some(combOpFreeVarsCompute.eval), | ||
) | ||
zeroFreeVars.merge(seqOpFreeVars).merge(combOpFreeVars) | ||
// the comb op can't refer to anything bound outside, so it can't have free variables | ||
zeroFreeVars.merge(seqOpFreeVars) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertion caught one place where the old implementation was wrong: AggFold
always added accumName
and otherAccumName
to the free variables, even though they are never free.
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @patrick-schultz and the rest of your teammates on Graphite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great change, just a question about the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See review comments above
8152579
to
d2d7604
Compare
d2d7604
to
f244cef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
laaavely
Before we can simplify the binding structure, we need to stop duplicating it all over the place. This PR rewrites
FreeVariables
so that it no longer needs special logic for particular nodes, hard coding binding structure (redundantly). To do this, it takes advantage of the newBindings
, which operates on aGenericBindingEnv
interface. It adds a new implementation of this interface specifically for computing free variables, then simply does a generic traversal of the IR using this custom binging environment.While I find the new implementation far simpler and more obviously correct than the old, I do expect it to further simplify once I'm able to start modifying the core binding structure.