Ignore line number node when counting expressions for inlining

[yuyichao]

Feels a little like a band-aid but I think this is the right logic.

Fix #13551.

[yuyichao]

CI failure is #11553 ....

[yuyichao]

CI restarted.

Also cc @vtjnash, could you have a look at this please?

[vtjnash]

it looks fine, although in general, it would be nice to be reducing the inlining threshold as that should result in slightly reduced compile times. this is likely only addressing a particular symptom of a different problem, not the actual problem.

[yuyichao]

this is likely only addressing a particular symptom of a different problem, not the actual problem.

Hmm. I don't think I quite understand what you mean. This should only revert the inlining threshold to the old value and that's AFAIK the problem. Or do you mean that the change in inlining threshold shouldn't have such a big effect?

For the test code I had above, one of the biggest problem is that *(::Complex{Float32}, ::Complex{Float32}) isn't inlined. Just looking at the function body, before the line number node filter change, the function body only has one expression. After the change, the function body has 20 expressions of which 19 are line numbers from random files (complex.jl, float.jl, essentials.jl to name a few). I'd be shocked if this alone is not enough to cause the problem

it looks fine, although in general, it would be nice to be reducing the inlining threshold as that should result in slightly reduced compile times.

Agree if we have a way to change that locally (for SIMD) and if non-inlined function don't have a huge performance penalty because of spatting (gc frame and boxing).

[yuyichao]

I'd be shocked if this alone is not enough to cause the problem

On the other hand, I would totally agree if the "actual problem" you meant is the duplicated line numbers in the function body. (only one of them would be useful anyway and they takes a lot of memory to store)

[yuyichao]

Should we at least also try to filter out the excessive line number node? On one hand they might be useful to construct a more complete backtrace (need to extend the debug info to do that) but on the other hand, they are not really accurate either. Even the current one can still be really confusing sometimes. For example, in the following code (put f and g in different functions can show this better),

f(x) = 1 ÷ x
g(x) = 1
@noinline k(x, y) = (x, y)
l(x) = k(f(x), g(x))

l(0)

Although the error is clearly thrown from f(x), the backtrace will show that it's inlined from g because g is inlined later and therefore overrides the debug info from f.

[vtjnash]

ok, it sounds like we should put back the line number filter, but now only remove nodes that don't match the file-name/line-numbers of the original function. or we would need to make our line number handling look more like the llvm DlInfo nodes, with explicit tracking of scope on each one.

[yuyichao]

I guess that would help but it will probably still effectively reduce the inlining threshold by up to a factor of 2?

[vtjnash]

it can be done in addition to this PR. the threshold computation is roughly 6 lines with an average of 16 nodes per line.

[yuyichao]

Thanks for the comments.

it can be done in addition to this PR

I'll take this as OK to merge. I've been carrying this patch in the my julia version and for the few benchmarks I've run, it seems that the performance is back to where it should be. Will merge some sometime tomorrow if there's no objections.

[tkelman]

If #13491 is going to be backported, then this should be too.

edit: and so should 2155f60, I think