Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

return matters in generated function lowering #25678

Closed
Keno opened this issue Jan 21, 2018 · 7 comments
Closed

return matters in generated function lowering #25678

Keno opened this issue Jan 21, 2018 · 7 comments
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior compiler:lowering Syntax lowering (compiler front end, 2nd stage)

Comments

@Keno
Copy link
Member

Keno commented Jan 21, 2018

Haven't tracked this down fully, but something is up with lowering:

@generated function code_llvm_helper(f, args...)
    NI.code_typed(f.parameters[1], Tuple{args...})[1].first
end
@generated function code_llvm_helper2(f, args...)
    return NI.code_typed(f.parameters[1], Tuple{args...})[1].first
end

julia> typeof(first(methods(code_llvm_helper)).generator.gen(nothing, Val{x->x}, [Bool]))
Expr

julia> typeof(first(methods(code_llvm_helper2)).generator.gen(nothing, Val{x->x}, [Bool]))
CodeInfo

This distinction matters quite a bit of course, since CodeInfo returns get treated specially.
Looks like the extra Expr in the first case is a LineNumberNode.

@Keno Keno added bug Indicates an unexpected problem or unintended behavior compiler:lowering Syntax lowering (compiler front end, 2nd stage) labels Jan 21, 2018
@vtjnash
Copy link
Member

vtjnash commented Jan 22, 2018

The result of code_typed isn’t valid as the return object for generated functions (we’re just sloppy about detecting this). Do you have an example with code_lowered?

@Keno
Copy link
Member Author

Keno commented Jan 22, 2018

Haven't tried, but it's a lowering transform that's being misapplied here, so I suspect the same will work.

@Keno
Copy link
Member Author

Keno commented Jan 22, 2018

Also, it would be nice if code_typed was valid for this, so I could feed my inference experiments to the code generator. It already does mostly work (prints an error, but then does compile the right code).

@vtjnash
Copy link
Member

vtjnash commented Jan 22, 2018

It's not valid because inference is an iterative algorithm, and this would be calling it recursively. It's also the wrong sort of return object (inferred code contains expressions that can't be passed to inference).

@Keno
Copy link
Member Author

Keno commented Jan 22, 2018

Well, I'm calling a different Inference's code_typed so from Inference's point of view I'm not calling it recursively.

@oxinabox
Copy link
Contributor

aslo came up in NHDaly/StagedFunctions.jl#8

@NHDaly
Copy link
Member

NHDaly commented Nov 12, 2019

Copied from that issue:

Apparently using an actual return code_info line, instead of just having the code_info as the final expression, prevents Julia lowering pass from wrapping the return value in an Expr(:block, ...), meaning it can be used by Julia to build the generated function.

Here is an example showing that difference:

With return:

julia> Meta.@lower @generated function f() return 2 end
...
    $(Expr(:meta, :nospecialize, :(@_2)))
    return 2
    Core._expr(:block, $(QuoteNode(:(#= REPL[107]:1 =#))), nothing)
    return %3
...

Without return:

julia> Meta.@lower @generated function f() 2 end
...
    $(Expr(:meta, :nospecialize, :(@_2)))
    Core._expr(:block, $(QuoteNode(:(#= REPL[108]:1 =#))), 2)
    return %2
...

As far as I can tell, it's been like this since Julia 0.7.

It looks to me like the lowering was attempting to wrap the return value in a QuoteNode (potentially to prevent returning a CodeInfo to have the above effect?), but that there's a bug that prevents that from happening if you return the expression instead of leaving it as the last line?

simeonschaub added a commit that referenced this issue May 10, 2021
Just explicitely check for `CodeInfo` objects when doing the
interpolation of the generated parts. I initially tried a more
complicated version of this, which avoided doing this check if there was
more than one generated part, but that's probably not necessary here.
simeonschaub added a commit that referenced this issue May 10, 2021
Just explicitely check for `CodeInfo` objects when doing the
interpolation of the generated parts. I initially tried a more
complicated version of this, which avoided doing this check if there was
more than one generated part, but that's probably not necessary here.
simeonschaub added a commit that referenced this issue May 19, 2021
Just explicitely check for `CodeInfo` objects and use an explicit
`return` in this case inside the `@generated` macro.

Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
KristofferC pushed a commit that referenced this issue Dec 7, 2021
Just explicitely check for `CodeInfo` objects and use an explicit
`return` in this case inside the `@generated` macro.

Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
(cherry picked from commit 92c84bf)
KristofferC pushed a commit that referenced this issue Jan 10, 2022
Just explicitely check for `CodeInfo` objects and use an explicit
`return` in this case inside the `@generated` macro.

Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
(cherry picked from commit 92c84bf)
LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Feb 22, 2022
…g#40778)

Just explicitely check for `CodeInfo` objects and use an explicit
`return` in this case inside the `@generated` macro.

Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Mar 8, 2022
…g#40778)

Just explicitely check for `CodeInfo` objects and use an explicit
`return` in this case inside the `@generated` macro.

Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
staticfloat pushed a commit that referenced this issue Dec 23, 2022
Just explicitely check for `CodeInfo` objects and use an explicit
`return` in this case inside the `@generated` macro.

Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
(cherry picked from commit 92c84bf)
vtjnash added a commit that referenced this issue Feb 9, 2023
Expose the demanded world to the GeneratedFunctionStub caller, for users
such as Cassette. If this argument is used, the uesr must return a
CodeInfo with the min/max world field set correctly.

Make the internal representation a tiny bit more compact also, removing
a little bit of unnecessary metadata.

Remove support for returning `body isa CodeInfo` via this wrapper, since
it is impossible to return a correct object via the
GeneratedFunctionStub since it strips off the world argument, which is
required for it to do so. This also removes support for not inferring
these fully (expand_early=false).

Also answer method lookup queries about the future correctly, by
refusing to answer them. This helps keeps execution correct as methods
get added to the system asynchronously.

This reverts "fix #25678: return matters for generated functions
(#40778)" (commit 92c84bf), since this
is no longer sensible to return here anyways, so it is no longer
permitted or supported by this macro.

Fixes various issues where we failed to specify the correct world.
maleadt pushed a commit that referenced this issue Feb 21, 2023
Expose the demanded world to the GeneratedFunctionStub caller, for users
such as Cassette. If this argument is used, the uesr must return a
CodeInfo with the min/max world field set correctly.

Make the internal representation a tiny bit more compact also, removing
a little bit of unnecessary metadata.

Remove support for returning `body isa CodeInfo` via this wrapper, since
it is impossible to return a correct object via the
GeneratedFunctionStub since it strips off the world argument, which is
required for it to do so. This also removes support for not inferring
these fully (expand_early=false).

Also answer method lookup queries about the future correctly, by
refusing to answer them. This helps keeps execution correct as methods
get added to the system asynchronously.

This reverts "fix #25678: return matters for generated functions
(#40778)" (commit 92c84bf), since this
is no longer sensible to return here anyways, so it is no longer
permitted or supported by this macro.

Fixes various issues where we failed to specify the correct world.
maleadt pushed a commit that referenced this issue Feb 22, 2023
Expose the demanded world to the GeneratedFunctionStub caller, for users
such as Cassette. If this argument is used, the user must return a
CodeInfo with the min/max world field set correctly.

Make the internal representation a tiny bit more compact also, removing
a little bit of unnecessary metadata.

Remove support for returning `body isa CodeInfo` via this wrapper, since
it is impossible to return a correct object via the
GeneratedFunctionStub since it strips off the world argument, which is
required for it to do so. This also removes support for not inferring
these fully (expand_early=false).

Also answer method lookup queries about the future correctly, by
refusing to answer them. This helps keeps execution correct as methods
get added to the system asynchronously.

This reverts "fix #25678: return matters for generated functions
(#40778)" (commit 92c84bf), since this
is no longer sensible to return here anyways, so it is no longer
permitted or supported by this macro.

Fixes various issues where we failed to specify the correct world.
maleadt pushed a commit that referenced this issue Feb 23, 2023
Expose the demanded world to the GeneratedFunctionStub caller, for users
such as Cassette. If this argument is used, the user must return a
CodeInfo with the min/max world field set correctly.

Make the internal representation a tiny bit more compact also, removing
a little bit of unnecessary metadata.

Remove support for returning `body isa CodeInfo` via this wrapper, since
it is impossible to return a correct object via the
GeneratedFunctionStub since it strips off the world argument, which is
required for it to do so. This also removes support for not inferring
these fully (expand_early=false).

Also answer method lookup queries about the future correctly, by
refusing to answer them. This helps keeps execution correct as methods
get added to the system asynchronously.

This reverts "fix #25678: return matters for generated functions
(#40778)" (commit 92c84bf), since this
is no longer sensible to return here anyways, so it is no longer
permitted or supported by this macro.

Fixes various issues where we failed to specify the correct world.
maleadt pushed a commit that referenced this issue Feb 24, 2023
Expose the demanded world to the GeneratedFunctionStub caller, for users
such as Cassette. If this argument is used, the user must return a
CodeInfo with the min/max world field set correctly.

Make the internal representation a tiny bit more compact also, removing
a little bit of unnecessary metadata.

Remove support for returning `body isa CodeInfo` via this wrapper, since
it is impossible to return a correct object via the
GeneratedFunctionStub since it strips off the world argument, which is
required for it to do so. This also removes support for not inferring
these fully (expand_early=false).

Also answer method lookup queries about the future correctly, by
refusing to answer them. This helps keeps execution correct as methods
get added to the system asynchronously.

This reverts "fix #25678: return matters for generated functions
(#40778)" (commit 92c84bf), since this
is no longer sensible to return here anyways, so it is no longer
permitted or supported by this macro.

Fixes various issues where we failed to specify the correct world.
vtjnash added a commit that referenced this issue Mar 22, 2023
Expose the demanded world to the GeneratedFunctionStub caller, for users
such as Cassette. If this argument is used, the user must return a
CodeInfo with the min/max world field set correctly.

Make the internal representation a tiny bit more compact also, removing
a little bit of unnecessary metadata.

Remove support for returning `body isa CodeInfo` via this wrapper, since
it is impossible to return a correct object via the
GeneratedFunctionStub since it strips off the world argument, which is
required for it to do so. This also removes support for not inferring
these fully (expand_early=false).

Also answer method lookup queries about the future correctly, by
refusing to answer them. This helps keeps execution correct as methods
get added to the system asynchronously.

This reverts "fix #25678: return matters for generated functions
(#40778)" (commit 92c84bf), since this
is no longer sensible to return here anyways, so it is no longer
permitted or supported by this macro.

Fixes various issues where we failed to specify the correct world.
vtjnash added a commit that referenced this issue Mar 24, 2023
Expose the demanded world to the GeneratedFunctionStub caller, for users
such as Cassette. If this argument is used, the user must return a
CodeInfo with the min/max world field set correctly.

Make the internal representation a tiny bit more compact also, removing
a little bit of unnecessary metadata.

Remove support for returning `body isa CodeInfo` via this wrapper, since
it is impossible to return a correct object via the
GeneratedFunctionStub since it strips off the world argument, which is
required for it to do so. This also removes support for not inferring
these fully (expand_early=false).

Also answer method lookup queries about the future correctly, by
refusing to answer them. This helps keeps execution correct as methods
get added to the system asynchronously.

This reverts "fix #25678: return matters for generated functions
(#40778)" (commit 92c84bf), since this
is no longer sensible to return here anyways, so it is no longer
permitted or supported by this macro.

Fixes various issues where we failed to specify the correct world.

Note that the passed world may be `typemax(UInt)`, which demands that
the generator must return code that is unspecialized (guaranteed to run
correctly in any world).

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Xnartharax pushed a commit to Xnartharax/julia that referenced this issue Apr 19, 2023
Expose the demanded world to the GeneratedFunctionStub caller, for users
such as Cassette. If this argument is used, the user must return a
CodeInfo with the min/max world field set correctly.

Make the internal representation a tiny bit more compact also, removing
a little bit of unnecessary metadata.

Remove support for returning `body isa CodeInfo` via this wrapper, since
it is impossible to return a correct object via the
GeneratedFunctionStub since it strips off the world argument, which is
required for it to do so. This also removes support for not inferring
these fully (expand_early=false).

Also answer method lookup queries about the future correctly, by
refusing to answer them. This helps keeps execution correct as methods
get added to the system asynchronously.

This reverts "fix JuliaLang#25678: return matters for generated functions
(JuliaLang#40778)" (commit 92c84bf), since this
is no longer sensible to return here anyways, so it is no longer
permitted or supported by this macro.

Fixes various issues where we failed to specify the correct world.

Note that the passed world may be `typemax(UInt)`, which demands that
the generator must return code that is unspecialized (guaranteed to run
correctly in any world).

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior compiler:lowering Syntax lowering (compiler front end, 2nd stage)
Projects
None yet
Development

No branches or pull requests

6 participants