abstract_apply: Don't drop effects of iterate'd calls
#47846
Conversation
We were accidentally dropping the effects of calls from `iterate` calls performed during abstract_iteration. This allowed calls that were not actually eligible for (semi-)concrete evaluation to go through that path anyway. This could cause incorrect results (see test), though it was usually fine, since iterate call tend to not have side effects. It was noticed however in #47688, because it forced irinterp down a path that was not meant to be reachable (resulting in a TODO error message). For good measure, let's also address this todo (since it is reachable by external absint if they want), but the missing effect propagation was the more serious bug here.
| @@ -114,6 +114,7 @@ Each (abstract) call to `iterate`, corresponds to one entry in `ainfo.each::Vect | |||
| """ | |||
| struct AbstractIterationInfo | |||
| each::Vector{CallMeta} | |||
| complete::Bool | |||
There was a problem hiding this comment.
Does this change is necessary for this PR or it is a separate fix on the inlining algorithm?
There was a problem hiding this comment.
This PR adds all the calls into the .each field (partly to look at the affects, but mostly for Cthulhu's benefit). Previously the inlining algorithm used the presence or absence of this info to determine whether or not we had the complete set of iteration items. Now we need to keep track of it separately.
| @@ -1482,7 +1492,7 @@ function abstract_iteration(interp::AbstractInterpreter, @nospecialize(itft), @n | |||
| # ... but cannot terminate | |||
| if !may_have_terminated | |||
| # ... and cannot have terminated prior to this loop | |||
| return Any[Bottom], nothing | |||
| return AbstractIterationResult(Any[Bottom], AbstractIterationInfo(calls, false), EFFECTS_UNKNOWN′) | |||
There was a problem hiding this comment.
EFFECTS_THROWS seems to be more consistent with the change on L1465 above?
| return AbstractIterationResult(Any[Bottom], AbstractIterationInfo(calls, false), EFFECTS_UNKNOWN′) | |
| return AbstractIterationResult(Any[Bottom], AbstractIterationInfo(calls, false), EFFECTS_THROWS) |
There was a problem hiding this comment.
At this point, we have proven that we cannot terminate, but I'm not sure we've proven that the state has fixpointed, meaning that our call effects cover the largest possible effect set the call could produce during its execution.
| effects = merge_effects(effects, ai_effects) | ||
| if info !== nothing |
There was a problem hiding this comment.
| effects = merge_effects(effects, ai_effects) | |
| if info !== nothing | |
| # merge effects of the `iterate` call | |
| effects = merge_effects(effects, ai_effects) | |
| # merge effects of call(s) with the iterated arguments | |
| if info !== nothing |
There was a problem hiding this comment.
It's not really the effects of the iterate call. It's more the effects of imprecision of the iterate call. The info.each, are all methods of iterate.
Co-authored-by: Shuhei Kadowaki <[email protected]>
| effects = merge_effects(effects, ai_effects) | ||
| if info !== nothing |
|
@nanosoldier |
|
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. |
We were accidentally dropping the effects of calls from `iterate` calls performed during abstract_iteration. This allowed calls that were not actually eligible for (semi-)concrete evaluation to go through that path anyway. This could cause incorrect results (see test), though it was usually fine, since iterate call tend to not have side effects. It was noticed however in #47688, because it forced irinterp down a path that was not meant to be reachable (resulting in a TODO error message). For good measure, let's also address this todo (since it is reachable by external absint if they want), but the missing effect propagation was the more serious bug here. (cherry picked from commit 2a0d58a)
We were accidentally dropping the effects of calls from
iteratecalls performed during abstract_iteration. This allowed calls that were not actually eligible for (semi-)concrete evaluation to go through that path anyway. This could cause incorrect results (see test), though it was usually fine, since iterate call tend to not have side effects. It was noticed however in #47688, because it forced irinterp down a path that was not meant to be reachable (resulting in a TODO error message). For good measure, let's also address this todo (since it is reachable by external absint if they want), but the missing effect propagation was the more serious bug here.