Use function type in fspec instead of value #320
Conversation
Co-authored-by: Valentin Churavy <[email protected]>
|
LGTM, but as I commented inline I'd prefer if we try to create nicer functions that accept function types instead of instances, and them try to upstream those. |
I concur, though since I presume we can't backport that all the way to 1.6 (Enzyme.jl's minimum test), I presume we'd need it here at minimum (in addition to upstream in later versions)? |
|
Yes normally we add compatibility shims for older Julia versions and then use the new APIs on new Julia versions. |
src/validation.jl
Outdated
| else | ||
| rt = Base.return_types(job.source.f, job.source.tt, interp)[1] | ||
|
|
||
| for m in Base._methods_by_ftype(tt, -1, job.source.world)::Vector |
| @@ -2,26 +2,36 @@ | |||
|
|
|||
| export InvalidIRError | |||
|
|
|||
| function get_method_matches(@nospecialize(job::CompilerJob)) | |||
There was a problem hiding this comment.
If this is the kind of definition we want to upstream, generalizing Base.methods to work with function types, wouldn't it be better to name it like that and pass a signature type instead of a CompilerJob?
| rt = Base.return_types(job.source.f, job.source.tt, interp)[1] | ||
| end | ||
| m = only(ms) | ||
| ty = Core.Compiler.typeinf_type(interp, m.method, m.spec_types, m.sparams) |
There was a problem hiding this comment.
Same here, maybe it's better to use a local version of return_types that accepts function types and work on upstreaming that.
| @@ -47,6 +47,11 @@ end | |||
| code_lowered(@nospecialize(job::CompilerJob); kwargs...) = | |||
| InteractiveUtils.code_lowered(job.source.f, job.source.tt; kwargs...) | |||
There was a problem hiding this comment.
code_lowered(::CompilerJob) uses job.source.f and is now broken.
There was a problem hiding this comment.
code_warntype below is also not adapted.
| @@ -83,11 +83,10 @@ end | |||
| kernel() = return | |||
|
|
|||
| ir = sprint(io->ptx_code_llvm(io, kernel, Tuple{})) | |||
| @test occursin(r"@.*kernel.+\(\)", ir) | |||
| @test any(occursin(r"@.*kernel.+\(\)", a) for a in split(ir, "\n")) | |||
There was a problem hiding this comment.
Why were these changes required?
|
Actually testing this with a GPU back-end revealed issues; I've commented on a couple, but there's other unadjusted uses of |
This is necessary to precompile function closures from the type alone, whose value may not be available yet.
Specifically, for use within Enzyme.
cc @vchuravy