Expr(:meta, :inline) sometimes ignored in @generated

[c42f]

While improving type inference in FixedSizeArrays, I've found that Expr(:meta, :inline) is sometimes ignored inside @generated functions in 0.5. Here's a somewhat minimal test case:

immutable Vec
    x::Int
    y::Int
end

@generated function should_be_inlined(a)
    quote
        $(Expr(:meta, :inline))
        # (A) This `nothing` appears from an elided bounds check in fixed size
        # array codegen; removing it explicitly fixes the problem which gives a
        # partial workaround.
        nothing
        # (B) @inbounds not required here but was needed in the original code
        # before reduction.  Removing it fixes the problem.
        @inbounds elements = (a.x, a.y)
        Vec(elements...)
    end
end

@inline function correctly_inlined(a)
    nothing
    @inbounds elements = (a.x, a.y)
    Vec(elements...)
end

foo1(a) = should_be_inlined(a)
foo2(a) = correctly_inlined(a)

println("\nCode for should_be_inlined()")
@code_native foo1(Vec(1,2))

println("\nCode for correctly_inlined()")
@code_native foo2(Vec(1,2))

For (A), the nothing may be replaced with various other things and the problem still occurs (for example, types eg Int), though some values like the empty tuple or a literal integer don't cause an issue.

I'm not quite sure whether the compiler is always meant to believe the inline metadata.

[timholy]

Added a 0.5 milestone, since #16895 suggests that this causes a massive performance problem in some cases. (See also #16128 (comment).)

[c42f]

I think I've got to the bottom of this: The compiler is emitting a Expr(:meta, :push_loc, filename) in julia-syntax.scm, which comes first in the block. This masks the inline meta inside inline_worthy() since popmeta!() only looks for the first :meta block.

Here's the evidence - I dumped the should_be_inlined body out by hacking inference.jl:

Expr(:block, # line 9,

Expr(:meta, :push_loc, :/home/cfoster/test_inference.jl)::Any, # line 10,  #### Oh dear :-(

Expr(:meta, :inline)::Any, # line 11,

M.nothing, # line 12,

Expr(:inbounds, true)::Any,

SSAValue(0) = Expr(:call, Core.getfield, SlotNumber(id=2), :x)::Int64,

SSAValue(1) = Expr(:call, Core.getfield, SlotNumber(id=2), :y)::Int64,

Expr(:inbounds, :pop)::Any, # line 13,


Expr(:return, Expr(:new, M.Vec, SSAValue(0), SSAValue(1))::M.Vec)::Any


)::Any

It's not yet clear to me what the right solution here is because http://docs.julialang.org/en/latest/devdocs/meta/ suggests that there should be one definitive meta block by design. I can hack around the problem by adding a few lines to inline_worthy() to look further than the first meta block, but this seems like a reasonably bad solution :-)

push_loc can be is added in two places: julia-syntax.scm (the apparent cause of the current problem; though commenting this out causes a build segfault) and in the inlineable() function inside inference.jl (commenting this out doesn't do anything to the output for the current test case)

Side note: I think the mess with the nothing vs empty tuple (), etc is an irrelevant distraction - the () is removed in a pass prior to inlining whereas the nothing is not, which leads to the inlining heuristics counting a different number of statements. In both cases, the explicit inline meta is ignored.

[JeffBezanson]

I think we have to make metadata-consuming passes more flexible; we can't assume there will be only one meta node, or that meta nodes will occur in a certain order. So yes, I'd probably start by looking at leading statements until the first non-meta one.

[c42f]

Ok, so if it seems right to modify the meta system, popmeta! should be updated for consistency. Currently it searches through all the code in a block (via findmeta_block) rather than stopping at the first non-meta statement, and there's also some logic for recursing into sub-blocks but the returned index doesn't reflect this recursion.

My current vague plan is to try updating the metadata handling so that it searches through all meta Exprs, but stops looking once it hits a non-meta statement (anything which is not an Expr(:meta,...), line number node, inbounds node, and a few others). BTW, as part of this I noticed that inbound nodes aren't part of inline_ignore.

Perhaps a more consistent story would involve replacing the other kinds of adhoc metadata (inbounds, line numbers?) with Expr(:meta, ...) alternatives?

[c42f]

replacing the other kinds of adhoc metadata

Here's a bit of a survey. In base we appear to have:

$ grep -h -R -o 'Expr( *:meta, *:[^,)]*'  | sort | uniq
Expr(:meta, :doc
Expr(:meta, :inline
Expr(:meta, :noinline
Expr(:meta, :pop_loc
Expr(:meta, :pure
Expr(:meta, :push_loc

$ grep -R pushmeta
# after cleanup
pushmeta!(ex, :propagate_inbounds)
pushmeta!(ex, :polly)

In the runtime, we have

$ grep -R '`(meta' julia-syntax.scm
# after cleanup
`(meta ret-type ,R)
`(meta push_loc ,fname)
`(meta pop_loc)

$ grep -R meta_sym
# ... suggests
e->head == line_sym || e->head == meta_sym || e->head == inbounds_sym ||
e->head == boundscheck_sym || e->head == simdloop_sym

To summarize

List of current :meta constructs

Expr(:meta, :doc
Expr(:meta, :inline
Expr(:meta, :noinline
Expr(:meta, :pure
Expr(:meta, :push_loc
Expr(:meta, :pop_loc
Expr(:meta, :propagate_inbounds
Expr(:meta, :polly
Expr(:meta, :ret-type # only in frontend

Out of these, pop_loc and push_loc are used somewhat differently, not being restricted to the start of a block.

Meta-like constructs and possible translation

Expr(:inbounds, true)
Expr(:inbounds, :pop)
Expr(:boundscheck, true)
Expr(:boundscheck, :pop)
Expr(:simdloop)
Expr(:line, i) # Obsoleted by LineNumberNode?
LineNumberNode # Required for efficiency?

It seems like we have two categories: those that apply to a julia function or block (doc, inline, noinline, pure, propagate_inbounds, polly, simdloop) and those that apply to a "meta block" which are pushed and popped to avoid introducing an actual block into the ast (push_loc, pop_loc, inbounds, boundscheck). It would be nice to have a systematic way to deal with the second class within the :meta mechanism. Perhaps Expr(:meta, :push, ...) and Expr(:meta, :pop) would work? Or would introducing an actual block be acceptable for these?

There's also the LineNumberNode which I assume is a special case due to having so many of them, and :line which I'm guessing is obsolete at this stage?

[JeffBezanson]

Yes, it would be good to make it uniform and use meta everywhere.

We can't introduce blocks in the IR; it only supports a single array of statements.

Yes, LineNumberNode is a special case because there are so many. :line Exprs shouldn't appear in IR. They only occur in surface ASTs, which e.g. get passed to macros, where everything is an Expr.

[JeffBezanson]

Also, I might have been wrong about it being ok to search until the first non-meta node. To be safe we should probably keep searching all statements.

[StefanKarpinski]

@c42f, are you going to put up a PR for this?

[c42f]

Well, making a PR seems like a good way to continue procrastinating instead of writing my juliacon talk ;-)

I'm happy to have a crack at this, but if someone else wants to claim it in the interests of just getting it fixed, that's also totally ok by me. Perhaps I should fix the immediate problem by just making popmeta!() search harder, and leave any larger cleanup of the meta system to later.

[JeffBezanson]

Yes, let's do the narrow fix, since it's getting to 0.5 crunch time.