Inlining statement cost: don't penalize call-Exprs that throw #23986
Conversation
| @@ -4756,6 +4756,8 @@ function statement_cost(ex::Expr, line::Int, src::CodeInfo, mod::Module, params: | |||
| return argcost | |||
| elseif f == Main.Core.arrayref | |||
| return plus_saturate(argcost, isknowntype(ex.typ) ? 4 : params.inline_nonleaf_penalty) | |||
| elseif f == Main.Core.throw | |||
There was a problem hiding this comment.
Should this just check the return type? Also I don't think it'll be good to return 0 (same below actually) since that'll change with linear IR.
There was a problem hiding this comment.
Should this just check the return type?
Seems like a better way to do it.
Also I don't think it'll be good to return 0
What else would you return, argcost? Do we really want to penalize the construction of arguments to the exception? This is mostly to make sure that simple functions containing a throw branch aren't penalized when it comes to inlining. The cost of the conditional is already counted, so this is only about any cost associated with actually throwing the error.
There was a problem hiding this comment.
What else would you return, argcost?
Yes.
Do we really want to penalize the construction of arguments to the exception?
The point is that we should be consistent. Hoisting an argument out to an SSAValue should have zero effect and all of them will soon (linear IR).
There was a problem hiding this comment.
I was about to make this change, but still have to ask: what if it's more than just hoisting? throw(InexactError(:gimme_pi, BigFloat, calculate_pi()))? That call to calculate_pi has no effect on ordinary runtime cost of the function, because it occurs only as an argument to throw.
There was a problem hiding this comment.
If you don't want that to be included in the cost then you should also not count anything in the error branch. As long as it's consistent it's fine.
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
Replaced by #30222 |
#22732 seems to have missed one other form of
throw:Expr(:call, throw, ...). Noticed while looking into performance of #23939.@nanosoldier
runbenchmarks(ALL, vs=":master")