Fix effects modeling for return_type #45299
Conversation
Keno
force-pushed
the
kf/rt_effect_free
branch
2 times, most recently
from
f51974c to
b8eb426
Compare
| elseif f === Tuple && la == 2 | ||
| tristate_merge!(sv, EFFECTS_UNKNOWN) # TODO | ||
| aty = argtypes[2] | ||
| ty = isvarargtype(aty) ? unwrapva(aty) : widenconst(aty) | ||
| if !isconcretetype(ty) | ||
| return CallMeta(Tuple, false) | ||
| return CallMeta(Tuple, EFFECTS_UNKNOWN, false) |
There was a problem hiding this comment.
Now we can remove the tristate_merge!(sv, EFFECTS_UNKNOWN) # TODO at L1640.
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| return CallMeta(Type, false) | ||
| return CallMeta(Type, EFFECTS_THROWS, false) |
There was a problem hiding this comment.
Should we call it EFFECTS_UNKNOWN here, since we don't know the method being called at all?
There was a problem hiding this comment.
(specifically, it might be one that specifies the world age, and thus possibly needs to happen specifically later)
There was a problem hiding this comment.
(specifically, it might be one that specifies the world age, and thus possibly needs to happen specifically later)
I'm not sure I'm following. Are you concerned that the version that takes a world age would get called with a future world-age (or possibly from pre-compilation), which would cause consistency to be tainted as well? Surely it should always at least terminate and be effect free for our purposes.
There was a problem hiding this comment.
I was mostly more considering that people could add other methods to it, hypothetically, and perhaps those wouldn't have the same effects limits. It is not a significant concern however.
There was a problem hiding this comment.
Perhaps, but by the same argument, someone could overwrite the method we're modeling above. I think going into Core.Compiler and extending and overriding methods is just caveat emptor.
| @@ -1749,15 +1739,13 @@ function abstract_call(interp::AbstractInterpreter, arginfo::ArgInfo, | |||
| tristate_merge!(sv, Effects()) # TODO | |||
There was a problem hiding this comment.
| tristate_merge!(sv, Effects()) # TODO |
I think this TODO is completed by abstract_call_opaque_closure's effects tracking above
There was a problem hiding this comment.
Not quite, because we don't account for the type constraints on the opaque closure type, which implicitly get evaluated on the call and return. I can probably just go ahead and fix that while I'm at it though.
In addition to the TODO placeholder that was taininting all effects for `return_type`, we were also accidentally picking up the effects of the function we were analyzing. This mostly didn't show up as an issue, because we were treating return_type as pure in optimization, but it could prevent deletion of unused functions by incorrectly tainting the effects.
Inlining had a special case for return_type that was not taking into account whether or not return_type was being called correctly. Since we already have the correct modeling in inference, remove the special case from inlining and simply have inference forward its conclusions.
JuliaLang/julia#45299 improved the effect inference on `Core.Compiler.return_type` callsite. This commit make our effect rendering consistent with it.
JuliaLang/julia#45299 improved the effect inference on `Core.Compiler.return_type` callsite. This commit make our effect rendering consistent with it.
Fix effects modeling for return_type
Effects modeling was bad in both directions (not precise in some cases, incorrect in others). This pair of commits refactors the code to make the effects modeling precise and then uses it in the optimizer to address the correctness issues.