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

inference: add missing limit-cycle poisoning #30098

Merged
merged 1 commit into from
Nov 27, 2018
Merged

inference: add missing limit-cycle poisoning #30098

merged 1 commit into from
Nov 27, 2018

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Nov 20, 2018

Seems to have been missing since the original implementation of IPO constant-prop.
It's impressive how long we got away with just poisoning the cache with Any
if there was a self-recursive call.

Fix #29923

@ararslan
Copy link
Member

Is it possible to add a test for this?

@martinholters
Copy link
Member

martinholters commented Nov 20, 2018

Can the type assertions in int.jl (#30032) be dropped then? (That would also act as a test, albeit a rather indirect one.)

@fredrikekre fredrikekre added backport pending 1.0 bugfix This change fixes an existing bug labels Nov 20, 2018
@martinholters
Copy link
Member

Can the type assertions in int.jl (#30032) be dropped then?

And the answer is ... almost. Those added in #30032 seem safe to drop, but

     function div(x::UInt128, y::UInt128)
-        return UInt128(div(BigInt(x), BigInt(y)))::UInt128
+        return UInt128(div(BigInt(x), BigInt(y)))
     end

still results in

julia> @inferred div(one(UInt128), one(UInt128))
ERROR: return type UInt128 does not match inferred return type Any
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] top-level scope at none:0

julia> @code_warntype div(one(UInt128), one(UInt128))
Body::UInt128
771 1%1 = invoke Base.BigInt(_2::UInt128)::BigInt                                                                            │ 
    │   %2 = invoke Base.BigInt(_3::UInt128)::BigInt                                                                            │ 
    │   %3 = Base.GMP.MPZ.tdiv_q::typeof(Base.GMP.MPZ.tdiv_q)                                                                   │╻ div
    │   %4 = invoke %3(%1::BigInt, %2::BigInt)::BigInt                                                                          ││
    │   %5 = invoke Base.UInt128(%4::BigInt)::UInt128                                                                           │ 
    └──      return %5  

So that's an instance which, without beneficially primed caches, is inferred as Any (and that result is cached and leads to the @inferred failing), but after some of the callees have been inferred to tighter results (and cached), inference invoked with @code_warntype also gives a tighter result?

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Nov 26, 2018

Is it possible to add a test for this?

Yes, it was just tricky to figure out. To answer my original question, we've gotten away with this for a long time because there's many fast-path efforts to ensure we don't get here in the majority of cases. In particular, the const-prop code needs to run into call-stack recursion that did not get observed without const-prop (e.g. would not be in the cache yet). The result of that recursion then gets set to Any, but it needs to be the case that without const-prop, we would have been able to figure out the result type anyways (even though without const-prop, we didn't even know this method would get called—but inside const-prop this method could affect the final answer!)

Here's the min repro of my above prose rendered as code:

_false = false
f() = _false ? g() : 3
g() = (h(:f); 4)
h(f) = getfield(Main, f)()
julia> Base.return_types(g, ())
1-element Array{Any,1}:
 Int64

julia> Base.return_types(f, ())
1-element Array{Any,1}:
 Any
julia> Base.return_types(f, ())
1-element Array{Any,1}:
 Int64

Seems to have been missing since the original implementation of IPO constant-prop.
It's impressive how long we got away with just poisoning the cache with `Any`
if there was a self-recursive call, but it requires a fairly convoluted
setup to trigger this case.

Fix #29923
@StefanKarpinski
Copy link
Sponsor Member

@vtjnash: Thank you for finding and fixing this and finding such a tricky repro/test case. ❤️

@KristofferC
Copy link
Sponsor Member

Does not backport cleanly. Please push backport to "backport-1.0.3" or make a rebased branch on top of that and I will backport the commits. @vtjnash

@KristofferC KristofferC mentioned this pull request Nov 28, 2018
61 tasks
@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Nov 28, 2018

I think you're likely missing a few commits (tested myself these should work):

* ff87181ebc N* - inference: set limited flag on entire cycle (10 seconds ago) <Jameson Nash>
* 6aa0a64d35 N* - interpreter: fix pref regression from #29795 (#29873) (5 minutes ago) <Jameson Nash>
* 266d6cde79 N* - partially address #29293 (5 minutes ago) <Jameson Nash>
* f375eb16e7 N* - inference: improve method caching (5 minutes ago) <Jameson Nash>

@KristofferC
Copy link
Sponsor Member

I don't have those commits:

➜  julia git:(backport-1.0.3) ✗ git cherry-pick -x ff87181ebc
fatal: bad revision 'ff87181ebc'

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Nov 28, 2018

oh, sorry, those hashes are my local branch. The commits are aaa9bd9, e1fbd0a, 92ab600, 439f798

@KristofferC
Copy link
Sponsor Member

Hmm, doesn't seem to work cleanly for me:

➜  julia git:(backport-1.0.3) ✗ git cherry-pick -x aaa9bd977815dc910be
[backport-1.0.3 70d5acdb74] inference: set limited flag on entire cycle
 Author: Jameson Nash <[email protected]>
 Date: Wed Oct 24 17:47:22 2018 -0400
 1 file changed, 10 insertions(+), 3 deletions(-)
➜  julia git:(backport-1.0.3) ✗ git cherry-pick -x e1fbd0a
error: could not apply e1fbd0a217... interpreter: fix pref regression from #29795 (#29873)
hint: after resolving the conflicts, mark the corrected paths

@KristofferC KristofferC mentioned this pull request Dec 30, 2018
53 tasks
@StefanKarpinski StefanKarpinski added triage This should be discussed on a triage call backport 1.0 and removed triage This should be discussed on a triage call labels Jan 31, 2019
@KristofferC
Copy link
Sponsor Member

Removing backport because it doesnt seem like it will happen.

@KristofferC KristofferC removed backport 1.0 triage This should be discussed on a triage call labels Jan 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants