Add --noinline startup option

[timholy]

Inlining (one of my favorite performance features of julia) can make debugging harder. This adds a --noinline startup option so you can turn it off. This was inspired by JuliaImages/Images.jl#232 (comment).

Demo (I temporarily commented out the line that fixed that issue): with regular startup,

julia> using Images, Color, FixedPointNumbers
# ambiguity warning suppressed

julia> img = Image(reinterpret(Gray{Ufixed16}, rand(Uint16, 5, 5)))
Gray Images.Image with:
  data: 5x5 Array{Images.ColorTypes.Gray{FixedPointNumbers.UfixedBase{UInt16,16}},2}
  properties:

julia> imgs = subim(img, :, :)
Gray Images.Image with:
  data: 5x5 SubArray{Images.ColorTypes.Gray{FixedPointNumbers.UfixedBase{UInt16,16}},2,Array{Images.ColorTypes.Gray{FixedPointNumbers.UfixedBase{UInt16,16}},2},(Colon,Colon),2}
  properties:

julia> map(ScaleAutoMinMax(RGB24), imgs)
ERROR: reinterpret: expected bits type as first argument
 in map! at /home/tim/.julia/v0.4/Images/src/map.jl:336
 in map at /home/tim/.julia/v0.4/Images/src/map.jl:311

With --noinline:

julia> using Images, Color, FixedPointNumbers
# ambiguity warning suppressed

julia> img = Image(reinterpret(Gray{Ufixed16}, rand(Uint16, 5, 5)))
Gray Images.Image with:
  data: 5x5 Array{Images.ColorTypes.Gray{FixedPointNumbers.UfixedBase{UInt16,16}},2}
  properties:

julia> imgs = subim(img, :, :)
Gray Images.Image with:
  data: 5x5 SubArray{Images.ColorTypes.Gray{FixedPointNumbers.UfixedBase{UInt16,16}},2,Array{Images.ColorTypes.Gray{FixedPointNumbers.UfixedBase{UInt16,16}},2},(Colon,Colon),2}
  properties:

julia> map(ScaleAutoMinMax(RGB24), imgs)
ERROR: reinterpret: expected bits type as first argument
 in reinterpret at base.jl:64
 in minfinite at /home/tim/.julia/v0.4/Images/src/algorithms.jl:164
 in call at /home/tim/.julia/v0.4/Images/src/map.jl:161
 in take at /home/tim/.julia/v0.4/Images/src/map.jl:347
 in map! at /home/tim/.julia/v0.4/Images/src/map.jl:336
 in map at /home/tim/.julia/v0.4/Images/src/map.jl:311
 in eval at no file

See how much easier it is debugging it this way? I especially like the fact that I wouldn't have had to use the "add @show statements"/"restart"/"reload Images (10s)" so many times.

[ivarne]

I like @jakebolewski's approach in #9294 somewhat better, because he provides a general compileropts() function, instead of this special case. The inline_worthy function also seems like a somewhat performance sensitive place to add a conditional. Is there any measurable performance regression?

[ViralBShah]

This is only a thought - just thinking about the way we specify arguments to the compiler, instead of --noinline and --check-bounds and such, should we go with the -f flags as clang does?

For example, -fnoinlining or -fdisable-inlining={true,false}. For bounds checking, do -fcheck-bounds={yes,no}

[timholy]

Even though I had briefly reviewed Jake's PR (excellent as always), I had forgotten about it again by the time I wrote this. I agree that just making the whole struct available to julia is cleaner. I can rebase this on his PR when it gets merged.

@ViralBShah, that would be fine with me. Can you more precisely clarify the benefit, though?

[timholy]

@ivarne, I forgot to address your question about performance. This conditional is much faster to evaluate than anything else in inline_worthy, so I can't imagine there will be a performance regression. The popmeta! and occurs_more lines are surely much more expensive.

When I've profiled, inference.jl rarely accounts for more than a modest fraction of package loading or compilation times. But of course different results will be obtained for different types of code.

[ViralBShah]

@timholy The thing I like about -f... is that Julia's compiler options start looking more familiar to someone familiar with C compiler switches. For example, -f switches are for feature-options, -m switches for machine-options. Kind of like -O for optimization that we have right now.

We only have a couple of these things right now, but they are likely to proliferate. In any case, this should not matter in this particular PR, and is perhaps a separate bikeshed issue.

[timholy]

I was trying to imagine what aspect of the code would be simplified, and couldn't come up with one. But if it's really to help users organize these options in their heads, that makes a certain amount of sense.

[rfourquet]

With #9349 this is a great day wrt debugging in Julia, thanks @timholy.

[timholy]

Yeah, all we need now is a debugger 😄. But thanks, I do hope these will make it much more pleasant.

[jakebolewski]

Small bikeshead, rename to --no-inline?

I don't think we have enough command line flags to warrant categorizing them (yet). I think we should actually try to avoid that.

[timholy]

Currently I agree that we don't yet have to categorize them, and like you I hope we never get there.

With regards to the naming of this option, I'd be happy to rename it. I chose a single word simply to make it similar to @noinline (and AFAIK we can't have dashes in macro names). What do others think?

[jakebolewski]

I don't really care about the name, maybe I should change --no-depwarn to --nodepwarn, I just found the latter to be less readable. It would be nice if these were consistent.

[timholy]

Wow, tough. I agree nodepwarn is not good. I'd probably vote for changing mine to no-inline. It seems more important to maintain consistency among startup options than it is between startup options and macros.

[JeffBezanson]

Exported functions should have the jl_ prefix.

[timholy]

Great suggestions, thanks everyone! I'll address all of these once @jakebolewski's #9294 gets merged.

[ivarne]

@StefanKarpinski

How about --inline=[yes|no] and it defaults to yes? That way when some other option becomes available, we don't have to change this in a backwards-incompatible way.

As this is a debug tool, I think it would be fine to just print pick a new name and issue a warning for the old option with directions for the new one. It's not like we want people to write scripts that require --no-inline, and forget about them?

[timholy]

It does also solve the naming problem, though. I worry that --inline=yes has confusing implications: is everything inlined? What about functions annotated with @noinline? Maybe --inline={default|no}? But then that's different from the others.

Maybe I'm just being pedantic, though. It is an attractive suggestion.

[ivarne]

We could also use on/off to make it more obvious that it is an heuristic.

[jakebolewski]

How about "inlining-pass" or "pass-inlining". That way you could eventually toggle the different optimization passes on and off from the command line. I actually have a branch that enables you to selectively turn on/off all optimization passes from type inference to the very last llvm pass but I don't think that is generally useful.

[lucasb-eyer]

How about "inlining-pass" or "pass-inlining"

Does it pass (i.e. skip) the inlining, or does it enable the inlining optimization pass?

[ghost]

Not entirely sure how feasible it is, but couldn't this be used to make the code coverage statistics more accurate? (#7541)

[timholy]

Definitely. This won't change the inlining of anything precompiled into the system, so it's not great for measuring coverage of base; but for packages, it should do exactly as you say.

[timholy]

OK, new version pushed. I went with the {yes|no} just for consistency with everything else.

One slight oddity: I moved the compileropts and associated type definition to c.jl. If folks think this is the wrong place, let me know. (Reason why it's there now: I needed it to come sooner in the boot process, before inference.jl, and because the type definition also uses types defined in c.jl, this was the earliest I could place it. Two other options I considered are base.jl and inference.jl.)

[tkelman]

Should we make a new file compileropts.jl and just put it wherever it needs to be in the sysimg order? I think compiler options are going to need a somewhat unified user-facing API sooner or later.

[timholy]

@tkelman, I wondered about that too, but it seems strange at this point. It's also a bit in the double-black-diamond territory to be monkeying directly with the compiler options, which leads me to think that perhaps inference.jl (which is already double-black-diamond territory 😄) might be the best place for this.

The AppVeyor failure seems unrelated. For the record, one of the failures is very strange (see also #6109 (comment)):

Exception: EXCEPTION_ACCESS_VIOLATION at 0x68d21b67 -- jl_profile_is_running at C:\projects\julia\usr\bin\libjulia.dll (unknown line)

at a point in the testing where I rather doubt the profiler is even active.

The other platform seemed to complete all of its tests successfully, but did emit the message

bash.exe: warning: could not find /tmp, please create!

and I wonder if that's the reason it declared the test a failure.

[timholy]

OK, I moved the whole compiler options to inference.jl. To me that now seems the best place, for now.

Once our tests turn green I'll merge this, barring other concerns.

[timholy]

@tkelman, any idea what's up with the AppVeyor builds?

[timholy]

@ArchRobison, any idea why this warning pops up (only with --inline=no):

julia> f(a) = a[2]
f (generic function with 1 method)

julia> g(a) = f(a)
g (generic function with 1 method)

julia> a = [5]
1-element Array{Int64,1}:
Warning: could not attach metadata for @simd loop.
 5

julia> g(a)
ERROR: BoundsError()
 in f at none:1
 in g at none:1
 in eval at no file

It pops up during compilation of the methods to display the value of a (it does not happen if I add a semicolon).

[tkelman]

I'll try to build this branch locally and see if I can reproduce the issues. The warning from bash.exe is normal. I've seen that segfault before, I think it's happening somewhere in GC or inference due to some kind of memory corruption or OOM situation. We can maybe try deleting sys.dll and see if that makes things any more reliable.

It's also a bit in the double-black-diamond territory to be monkeying directly with the compiler options

Well by modifying compiler options what I meant was things we're currently doing via command-line switches. I'd personally love to see those all go away and be replaced by in-Julia API's.

[timholy]

Thanks, @tkelman.

Regarding Julia APIs, I worry that modifying cpu_target on-the-fly is not exactly something we should support. But perhaps you're right that some of the others could be tweaked at runtime. If this is something you're interested in exploring, I'll move it to a compileropts.jl file as you suggest.

[tkelman]

Yeah, some are obviously more suited for tinkering by the user than others. And there's the race condition concern, especially when we start getting multiple Julia threads.

I don't know if or when I'll be the one to get to that kind of thing, but this and the various other PR's that add or modify any command-line flags (#9482 and others) pretty much all conflict with one another. Maybe it's not such a big deal, but a little coordination of the changes might be worthwhile.

[timholy]

@ArchRobison, I think I figured this out: by uncommenting these lines, I found that the warning is being produced during compilation of mapreduce_seq_impl at reduce.jl:217. That function has an explicit @simd in it, and a function call that will not be inlined when --inline=no. So it seems that your warning is working perfectly.

[tkelman]

Also speaking of conflicts I'm not sure what the conflict here is (looks like you last rebased just a few hours ago?), so it might be that AppVeyor is merging something funky or old when it does

git fetch -q origin +refs/pull/9354/merge:
git checkout -qf FETCH_HEAD

For instance I don't see it executing the line from 592f3d5, and the headline for which commit it's building says 5a677d5 which doesn't look right.

edit: also this branch passes tests fine locally on both win32 and win64 when I check out e5d3a5f

[timholy]

Yes, I've amended-and-forced-pushed this several times now. Maybe AppVeyor has trouble with forced pushes?

Anyway, it sounds like this can be merged. That will let Jake get on with #9482, so I'll do it now. @jakebolewski, if as part of your changes, you want to move the compiler options material I moved from util.jl to inference.jl into a new compileropts.jl file, be my guest (see discussion above).

[tkelman]

Maybe AppVeyor has trouble with forced pushes?

Not usually. But it might be getting wacky information from GitHub right now for some reason, are you seeing the "We can't automatically merge this pull request" status like I am? We could ping Feodor and let him know something strange is happening here if we feel the need to.

[timholy]

I just did a rebase. Will wait again to see what happens with testing.

[tkelman]

Worked this time (bit of a queue). Should we merge?

[ArchRobison]

Thanks @timholy for the note about the @simd warning. The warning relates to the hack I used to pass LLVM metadata from Julia to the custom LLVM pass src/llvm-simdloop. If it becomes a regular nuisance, I can look into improving the hack.