inference: create a separate type for doing optimizations #23276
Conversation
vtjnash
force-pushed
the
jn/split-out-optpass
branch
from
7cc97e7 to
8ad6424
Compare
| "invalid age range update") | ||
| nothing | ||
| end | ||
| function update_valid_age!(min_valid::UInt, max_valid::UInt, sv::OptimizationState) |
There was a problem hiding this comment.
why isn't the mutated state input the first argument to this function?
There was a problem hiding this comment.
with what? that's an inconsistency with Julia API recommendations (aka conventions) everywhere else
There was a problem hiding this comment.
then
why isn't the mutated state input the first argument to this function
in the rest of the file?
There was a problem hiding this comment.
probably convention or something
There was a problem hiding this comment.
These conventions are not really important for internal functions.
There was a problem hiding this comment.
Re: the confused emojis: yes it's nice to follow the conventions, but we have more important things to spend time on than reviewing the design of every private function that nobody will ever see. Priorities, priorities, priorities.
There was a problem hiding this comment.
most critical thing to fix right now, no, but it is actively confusing for API's to be backwards for anyone who's debugging around this file
vtjnash
force-pushed
the
jn/split-out-optpass
branch
from
8ad6424 to
cfcc208
Compare
base/inference.jl
Outdated
| @@ -2742,19 +2806,31 @@ end | |||
|
|
|||
| #### helper functions for typeinf initialization and looping #### | |||
|
|
|||
| # scan body for the value of value of the largest referenced label | |||
There was a problem hiding this comment.
"value of value" intended?
vtjnash
force-pushed
the
jn/split-out-optpass
branch
from
cfcc208 to
77fa892
Compare
base/inference.jl
Outdated
| params::InferenceParams | ||
| function OptimizationState(frame::InferenceState) | ||
| s_edges = frame.stmt_edges[1] | ||
| if s_edges=== () |
base/inference.jl
Outdated
| s_edges = frame.stmt_edges[1] | ||
| if s_edges=== () | ||
| s_edges = [] | ||
| frame.stmt_edges[1] = s_edges |
There was a problem hiding this comment.
Is there any way around mutating frame? That's a pretty strange thing for a constructor to do.
There was a problem hiding this comment.
I could make it part of the initialization for frame, but it's just more sensible to handle the lazy initialization here.
base/inference.jl
Outdated
| @@ -3365,40 +3460,44 @@ function optimize(me::InferenceState) | |||
| type_annotate!(me) | |||
|
|
|||
| # run optimization passes on fulltree | |||
| force_noinline = false | |||
| force_noinline = true | |||
There was a problem hiding this comment.
We've been miscomputing this; but fortunately, since it only affects the cached=no case, they're haven't been any consumers of the data.
There was a problem hiding this comment.
It doesn't make sense to me that force_noinline would default to true. It's supposed to be true only for functions declared @noinline. In fact with this change I think it's now always true? Am I missing something?
There was a problem hiding this comment.
Regardless of what the correct default is here, this should not be included in a large, unrelated pull request. If this is wrong, it should be changed in a separate individual PR with a commit message that explains what was wrong and why the change is correct.
There was a problem hiding this comment.
It's a bit of an abuse of the variable name (phrasing it in the positive would be more accurate), since it should be dependent on optimize=true. We use this later to decide whether to compute the inlining optimization for the function. While instead that operation should be separated out into optimization, like the rest of this PR is working towards enforcing.
There was a problem hiding this comment.
If you're changing the meaning of this to the complete opposite, can you please change the name of the variable?
Most fields of InferenceState aren't valid during optimization, so the goal is to reflect that in the structure of the types Similarly, doing optimization operations during inference would be invalid, so this helps distinguish those cases as well. Finally, this makes is possible for non-InferenceState-initiated IR passes (e.g. external to typeinf) to make uses of these passes.
vtjnash
force-pushed
the
jn/split-out-optpass
branch
from
77fa892 to
45d0650
Compare
Most fields of InferenceState aren't valid during optimization,
so the goal is to reflect that in the structure of the types.
Similarly, doing optimization operations during inference would be
invalid, so this helps distinguish those cases as well.
Finally, this makes is possible for non-InferenceState-initiated IR passes
(e.g. external to typeinf) to make uses of these passes.
(Also, since it may not be eminently clear, note that the other goal of starting this is to eventually separate anything that takes an InferenceState into one file, and anything that takes OptimizationState into a second file)