-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Conversation
Is it possible to add a test for this? |
Can the type assertions in int.jl (#30032) be dropped then? (That would also act as a test, albeit a rather indirect one.) |
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 |
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
@vtjnash: Thank you for finding and fixing this and finding such a tricky repro/test case. ❤️ |
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 |
I think you're likely missing a few commits (tested myself these should work):
|
I don't have those commits:
|
Hmm, doesn't seem to work cleanly for me:
|
Removing backport because it doesnt seem like it will happen. |
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