-
-
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
Fix effects modeling for return_type #45299
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.