-
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
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
d2a4a9b
to
39d62b0
Compare
23232cd
to
6f3042d
Compare
39d62b0
to
56e3cee
Compare
6f3042d
to
62c51bc
Compare
56e3cee
to
17107d2
Compare
62c51bc
to
d658770
Compare
17107d2
to
9679809
Compare
d658770
to
8f664ef
Compare
9679809
to
a36d841
Compare
8f664ef
to
b3cce7a
Compare
a36d841
to
ebd7176
Compare
b3cce7a
to
0544342
Compare
Security Assessment
This change has no security impact as it just moves code around.
(Reviewers: please confirm the security impact before approving)