more inlining

[vtjnash]

This pull requests adds inlining and improved call site type specialization. It separates function calls of type unions and provides support for multi-line inlining. (It also attempts to inline Union, but that part is causing the tests to fail)

While the standard inlining heuristic has not been updated, you can test the multiline inlining by forcing a function to be inlined by adding an :inline keyword:

function abs(x) :inline
    if x > 0
        x
    else
        -x
    end
end

[timholy]

Fantastic! This is something I've been wanting for a long time. Amazing work.

This will fix both #3030 and #1106.

[lindahua]

This is awesome! It may hopefully address many of the challenges we have been facing (e.g. make map, broadcast and reduce much more performant). I am looking forward to this.

[quinnj]

How about going C99 style and making inline a full keyword to be used in place of function?

This is exciting stuff!

[StefanKarpinski]

I think the :inline thing is just a temporary hack to avoid messing with the parser for now. I've also brought up the possibility of call-site inline annotation in addition to the traditional definition-site inline annotation.

[staticfloat]

Is there any documentation on the :inline thing? I'd like to play around with attaching data to functions to see if it would be as natural as I'd want it to be. (Useful for documentation, descriptions of tests for codespeed, etc....)

[JeffBezanson]

I suggest using macro syntax instead of adding new keywords. This allows @inline function f() to declare a function as inline, and @inline f(x) to do call site inlining. The call site version will require more work, since we'd need something like an inline expression wrapping the call. For the function def version, I'd like to add a meta expression head, for adding extra declarations and info to functions more generally. Then the @inline macro can simply insert (meta inline) as one of the first statements in the function.

[vtjnash]

@JeffBezanson The @inline macro sounds like it is doing the same as the current implementation, other than wrapping it in an Expr type?

How about:
@inline f(y) = @inline g(y)

becomes
Expr(:function, Expr(:call, :f, :y), Expr(:block, Expr(:meta, :inline), Expr(:call_inline, :g, :y)))

[JeffBezanson]

Yes. But the value of wrapping it in the meta expr is you can easily identify all such non-code things. I think eventually we should use it for line numbers too, but that's too disruptive right now.

Adding :call_inline is not workable since so, so many things look for call exprs. Expr(:inline, Expr(:call, :g, :y)) or Expr(:withmeta, Expr(:call, :g, :y), :inline) is probably easier to work with. Note it should not use meta, since those should never contain executing code.

[vtjnash]

I like the :withmeta syntax. And it should be a lot less work than trying to fixup all locations of :call, while being more general.

Perhaps meta should be part of an Expr: then we could write the following:
Expr(:call, :g, :y; meta=(:inline,))

Expr(:function, Expr(:call, :f, :y; meta=(:inline,)), Expr(:block, Expr(:call, :g, :y; meta=(:inline,))))

[vtjnash]

@JeffBezanson bump. should I add a meta field to every Expr? It could probably have any of the following layouts:
(:line, 123, (:file, "hello world.jl", (:inline, :always)))
{(line,123),(file,"hello world.jl"),(:inline,:always))
{:line=>123, :file=>"hello world.jl", :inline=>:always}

Or how would you prefer to handle this?

[JeffBezanson]

For now, let's remove the :inline thing, deal with declarations later, and separate the inlining heuristic into its own function. Then we can experiment with tweaking it. (after all, inline declarations should not be required to get good performance)
Then we can probably merge the multi-line inlining part pretty easily.

[StefanKarpinski]

^^ pro move: break the change into smaller, less disruptive pieces.

On Thursday, August 15, 2013, Jeff Bezanson wrote:

For now, let's remove the :inline thing, deal with declarations later,
and separate the inlining heuristic into its own function. Then we can
experiment with tweaking it. (after all, inline declarations should not be
required to get good performance)
Then we can probably merge the multi-line inlining part pretty easily.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3796#issuecomment-22685797
.

[Keno]

@vtjnash Would you mind rebasing this? I wanted to play with inling a bit, but didn't want to duplicate any work.

[vtjnash]

@loladiro @JeffBezanson this has been rebased. please test and merge when you are happy

[JeffBezanson]

Currently this patch undoes my optimization for many-argument operators like + (issue #4374). This probably happened as part of the rebase. This has to be fixed.

Ideally the multi-statement inlining and union-splitting would be separate patches, as well.

[vtjnash]

Yeah, it was partially that, and partially that I was being too aggressive at creating local variables while inlining so that optimization could no longer be applied. I fixed that stuff, and reverted the inlining_pass function for simplicity (and because it seems the order of the expanding apply vs calling inlineable really matters).

I also found that I was accidentally inlining some rather large functions, which is why the test were so slow. That's fixed now also.

[vtjnash]

@JeffBezanson bump. i think i've removed all the controversial stuff. now it just does inlining based on call1 and inline_worthy, nothing more.

OK to merge? or do you have other suggestions first?

[ViralBShah]

Why does github not like automatically merging this? Some rebasing required?

[vtjnash]

Probably not. I think github is just refusing to do the file merge caused by b4fa861

[timholy]

I'm assuming that at this point this is 0.4 material. When it does come to the fore, here's one vote for considering restoring the :inline keyword. The reasons are an elaboration of my suspicion about notions of "big functions" and "little functions" (see above):

• The heuristic is very crude. For example, my version that did get inlined presumably was compiled by LLVM to the same thing as for the version that wasn't being inlined. The distinction was that the original version had (due to auto-generation) lots of statements like if 1 == 3; nothing ; elseif 2 == 3; nothing; elseif 3 == 3; # do something; end. Those add to the expression-count, and hence make it look "big" by the heuristic that's in place, but they should disappear once LLVM has had a chance to do its magic. I was able to add a feature to Cartesian that allowed it to generate prettier and more compact expressions---which is not a bad thing---but it's important to recognize that fundamentally this changed nothing, yet had a dramatic improvement on performance due to the decision about whether to inline.

  One might improve the heuristic, but at what point does it simply become its own compiler?

• In general, for the vast majority of iterators I suspect that next and done should probably be inlined. How would you feel if we didn't inline them in for i = 1:1000; # do something; end? For a multidimensional iterator, usually only the inner-loop index will need to be incremented, so it's exactly analogous to this case. Moreover, this is just as true for a 6d iterator as it is for a 2d iterator. But iteration in 6d will necessitate a next function body at least 3x as long as a 2d iterator, and might easily cross threshold for not being inlined by whatever heuristic we use. Nevertheless, in reality it's no less worthy of inlining.

I suspect there's some justifiable reluctance to give users too much control over inlining: witness the number of recommendations floating around to avoid overusing inline in C. I agree that you don't want to inline everything into everything. But it's probably also fair to say that one could view Cartesian partially as an exercise in circumventing the problems that arise from not being able to inline things that you really, really need to inline. Some things about the language will be a lot cleaner if we don't have to worry about whether something gets inlined.

All that said, in practice many of our current problems will be solved even by this version. I'd much rather see this merged than to have it held up by a lack of consensus about an :inline keyword; please don't misinterpret these comments as suggesting otherwise.

[lindahua]

+1 for @timholy's argument.

I have often found it quite frustrated to see the performance penalty caused by failure of inlining. In performance-demanding settings (which is not uncommon in scientific computing), this virtually forces me to use macros or more verbose codes instead of more elegant abstractions (e.g. iterators).

I support merging this soon and then debating how we may provide ways for developers to specify what to inline.

[JeffBezanson]

Why 5?

[vtjnash]

The return type and param type arguments don't run in enclosing_ast as
expected by inlineable, so we need to skip them also

On Wednesday, March 26, 2014, Jeff Bezanson notifications@github.com
wrote:

In base/inference.jl:

 if is_known_call(e, Core.Intrinsics.ccall, sv)

•    i0 = 3
  

•    i0 = 5
  

Why 5?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3796/files#r10987984
.

[JeffBezanson]

Did I just rebase this? Yes. What can that possibly mean? Who knows.

[timholy]

Don't mind me, I'll just stand here in the corner whistling, peering over my shoulder occasionally.

[jiahao]

🍬

[tknopp]

A big "thank you" for this wonderful PR.