-
Notifications
You must be signed in to change notification settings - Fork 248
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
[query] Lowering + Optimisation with implict timing context #14731
base: ehigham/stateless-backend
Are you sure you want to change the base?
[query] Lowering + Optimisation with implict timing context #14731
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Example output in logs:
|
try | ||
TypeCheck(ctx, ir) | ||
catch { |
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.
This is the major change in this class - move TypeCheck
here rather than call in Optimize
.
Otherwise, this is mostly whitespace changes
ctx.time { | ||
val uses = mutable.HashMap.empty[Name, (Int, Int)] | ||
val nestingDepth = NestingDepth(ctx, ir0) | ||
IRTraversal.levelOrder(ir0).foreach { |
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.
One change in this file is to only insert into uses
when there's a ReleationalRef
.
The test for shouldForward
is adjusted for the case when uses
is empty
try | ||
TypeCheck(ctx, ir) | ||
catch { |
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.
Moving typecheck here also to better mirror ForwardLets
.
Probably these two optimisations can be combined.
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.
I strongly recommend reviewing this PR with whitespace changes hidden. Adding ctx.time
adds a level of indentation.
} | ||
} | ||
def apply(ctx: ExecuteContext, ir0: BaseIR): NestingDepth = | ||
ctx.time { |
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.
This + whitespace changes
runOpt(ForwardLets(ctx), iter, "ForwardLets") | ||
try | ||
|
||
private[this] val optimizations: Array[(ExecuteContext, BaseIR) => BaseIR] = |
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.
A theme of these changes is making these functions of the form below so they compose nicely and have explict ctx threading
(ExecuteContext, BaseIR) => BaseIR
def apply(ctx: ExecuteContext, ir: BaseIR): BaseIR = | ||
ctx.time(recur(ctx, ir)) |
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.
Needed a top level apply method that wasn't recursive
I also took the libery of simplifying visitNode
as it always called Simplify
def apply(ir: BaseIR, ctx: ExecuteContext, passesBelow: LoweringPipeline): BaseIR = | ||
ctx.time { | ||
def execute(value: BaseIR, letsAbove: Map[Name, IR]): IR = | ||
ctx.time { |
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.
Added timing to evaluating each let
} | ||
|
||
trait IRState { | ||
abstract class IRState(implicit E: sourcecode.Enclosing) { |
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.
Need to pass context from the parent class otherwise the timings from verify
won't include information about which condition is being verified`
}), | ||
aggSigs.map(_.state), | ||
def apply(ir: BaseIR, ctx: ExecuteContext, passesBelow: LoweringPipeline): BaseIR = | ||
ctx.time { |
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.
This lead to whitespace changes. Sorry.
@@ -23,16 +23,16 @@ final class IrMetadata() { | |||
} | |||
} | |||
|
|||
trait LoweringPass { | |||
abstract class LoweringPass(implicit E: sourcecode.Enclosing) { |
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.
Same as IRState
I'm going to change from |
fb23e65
to
7e96961
Compare
009606e
to
96d5b50
Compare
7e96961
to
af28777
Compare
96d5b50
to
96def90
Compare
af28777
to
9cf65a9
Compare
96def90
to
f98cad5
Compare
9cf65a9
to
40d15ac
Compare
f98cad5
to
6839573
Compare
1825e8e
to
14bf960
Compare
b9424b9
to
454ea3b
Compare
14bf960
to
1c00118
Compare
454ea3b
to
0b3c0aa
Compare
1c00118
to
0c31d9d
Compare
0b3c0aa
to
85840f9
Compare
0c31d9d
to
5b6ce26
Compare
d25bb41
to
4da26d3
Compare
@@ -20,7 +20,6 @@ object InterpretableButNotCompilable { | |||
case _: MatrixToValueApply => true | |||
case _: BlockMatrixToValueApply => true | |||
case _: BlockMatrixCollect => true | |||
case _: BlockMatrixToTableApply => 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.
fruitless test
@@ -44,7 +43,6 @@ object Compilable { | |||
case _: TableToValueApply => false | |||
case _: MatrixToValueApply => false | |||
case _: BlockMatrixToValueApply => false | |||
case _: BlockMatrixToTableApply => false |
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.
fruitless test
|
||
trait IRState { | ||
|
||
val rules: Array[Rule] |
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.
A Rule
is a predicate. The new RichPredicate
class adds a cleaner way of combinding predicates.
5b6ce26
to
f12b826
Compare
4da26d3
to
c49d75b
Compare
f12b826
to
4387cc5
Compare
c49d75b
to
7917907
Compare
4387cc5
to
d14286b
Compare
7917907
to
5a64ac4
Compare
d14286b
to
1e00ba8
Compare
5a64ac4
to
1968227
Compare
1e00ba8
to
e1c32bc
Compare
1968227
to
fd9bde2
Compare
e1c32bc
to
9768f54
Compare
fd9bde2
to
9f5ce3f
Compare
9768f54
to
f5f4020
Compare
9f5ce3f
to
6195b9d
Compare
f5f4020
to
00b7f03
Compare
6195b9d
to
fac7506
Compare
00b7f03
to
d2a4a9b
Compare
fac7506
to
23232cd
Compare
Security Assessment
This change has no security impact as it just moves code around.
(Reviewers: please confirm the security impact before approving)