[compiler] replace Begin with Let #14068
Conversation
patrick-schultz
force-pushed
the
end-of-begin
branch
from
1ff9a5c to
f6fe074
Compare
patrick-schultz
force-pushed
the
end-of-begin
branch
from
c829069 to
8499e94
Compare
# Conflicts: # hail/src/main/scala/is/hail/expr/ir/Children.scala # hail/src/main/scala/is/hail/expr/ir/Copy.scala # hail/src/main/scala/is/hail/expr/ir/Emit.scala # hail/src/main/scala/is/hail/expr/ir/TypeCheck.scala
patrick-schultz
force-pushed
the
end-of-begin
branch
2 times, most recently
from
8d8699b to
e4acd13
Compare
patrick-schultz
force-pushed
the
end-of-begin
branch
from
e4acd13 to
935c2a6
Compare
patrick-schultz
force-pushed
the
end-of-begin
branch
from
19164ad to
e5ee4cd
Compare
There was a problem hiding this comment.
Great! One less IR node :)
My comments are mostly around emitLetBindings - I can't dont really understand what it's doing and feels too low-level. Keeping track of three ints may be too much for me.
You could consider using the mark flag on BaseIR to skip emitting certain nodes - that might make the logic a bit easier to read.
| FastSeq[ParamType](classInfo[Region]), | ||
| UnitInfo, | ||
| ) | ||
| var newEnv = env |
There was a problem hiding this comment.
This env needs to be restricted to fields and formal parameters only.
We may not bind any locals at present, but I can imagine if we did and they were used across a split, we'd get a nasty runtime error
There was a problem hiding this comment.
Also, make this a param of emitChunkWrapped - this ref copy just looks ugly
There was a problem hiding this comment.
This env needs to be restricted to fields and formal parameters only. We may not bind any locals at present, but I can imagine if we did and they were used across a split, we'd get a nasty runtime error
The env must have this property already, as we currently emit parts of the ir into separate methods, and simply pass in the current env. See for instance Emit.emitInSeparateMethod.
Also, make this a param of
emitChunkWrapped- this ref copy just looks ugly
I'm not sure what you mean. The newEnv var is so that we can smuggle out the update env from emitChunk out of voidWithBuilder.
| chunkStart: Int, | ||
| pos: Int, |
There was a problem hiding this comment.
What's pos for? There seems to be three ints we're tracking at once when wouldn't two be enough to define the window of the bindings to emit in one group?
There was a problem hiding this comment.
Added explanatory comments.
| val (curName, curIR) = let.bindings(pos) | ||
|
|
||
| // skip over unused streams | ||
| if (curIR.typ.isInstanceOf[TStream] && !uses.contains(curName)) { |
There was a problem hiding this comment.
We've testing for some uses in more than two places, can these be folded into one?
There was a problem hiding this comment.
I think only by using an array builder to hold the current chunk, instead of just slicing into the bindings seq. uses is just a set, so testing for membership is fast.
patrick-schultz
force-pushed
the
end-of-begin
branch
from
aa5919d to
e6a3710
Compare
| import is.hail.types.virtual.TVoid | ||
|
|
||
| object IsPure { | ||
| def apply(x: IR): Boolean = x match { |
There was a problem hiding this comment.
We don't actually treat Die as side effecting. For instance, in
GetField(
MakeStruct(foo = I32(5), bar = Die())
"foo")
we will prune "bar", even though it changes the program from dying to returning a value.
Formally, our semantics is slightly non-deterministic: there is at most one correct value result, and any number of correct exception results, and running the program is allowed to produce any of those behaviors.
There was a problem hiding this comment.
Only if it violates the semantics of the given IR. We intentionally use a semantics for which this is not a bug. Preserving all potential errors would be a huge burden on the optimizer. For instance, one important optimization is pushing computation to happen after a filter. But if the computation could possibly error (eg divide by zero), then that's an invalid optimization. Or if a big struct is computed, then only some of its fields are used, we'd be forced to compute all the unused fields in case they error.
Remove the
Beginnode, as its behavior can now be represented by theLetnode. Besides removing redundant nodes, this will also make the new ssa-style text representation simpler.The
Beginnode emmitter performed method splitting, emitting groups of 16 children in seperate methods. This preserves that behavior by doing a similar optimization in theLetemitter. This is a significant change in how we split generated code into methods, so we should watch out for how this affects things.