irinterp: Fix accidentally introduced deletion of effectful statement #49750
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I moved around some code in #49692 that broadened the replacement of statements by their const results. This is fine for how we're currently using irinterp in base, because we're requiring some fairly strong effects, but some downstream pipelines (and potentially Base in the future) want to use irinterp on code with arbitrary effects, so put in an appropriate check.
aviatesk
reviewed
May 11, 2023
| @@ -2544,6 +2544,7 @@ end | |||
| function abstract_eval_statement(interp::AbstractInterpreter, @nospecialize(e), vtypes::VarTable, sv::InferenceState) | |||
| if !isa(e, Expr) | |||
| if isa(e, PhiNode) | |||
| add_curr_ssaflag!(sv, IR_FLAG_EFFECT_FREE) | |||
There was a problem hiding this comment.
Why is this needed? This function works on InferenceState and thus irinterp will never use it. The ordinary inference may see PhiNode somehow?
There was a problem hiding this comment.
We made it legal in untyped code, but it is true that we may need to add the flag in ssa conversion also.
There was a problem hiding this comment.
I wonder if there is any actual users of this PhiNode handling within untyped code inference. And it sounds a good idea to add the flag during SSA conversion.
There was a problem hiding this comment.
I wonder if there is any actual users of this PhiNode handling within untyped code inference.
Diffractor uses it.
And it sounds a good idea to add the flag during SSA conversion.
I think we already set this since stmt_effect_flags knows about it, but we've of course been moving to not
calling that function quite as often.
aviatesk
approved these changes
May 11, 2023
Keno
added a commit
that referenced
this pull request
May 12, 2023
This is yet another followup to #49692 and #49750. With the introduced change, we kill the CFG edge from the basic block with the discovered error to its successors. However, we have an invariant in the verifier that the CFG should always match the IR. Turns out this is for good reason, as we assume in a number of places (including, ironically in the irinterp) that a GotoNode/GotoIfNot terminator means that the BB has the corresponding number of successors in the IR. Fix all this by killing the rest of the basic block when we discover that it is unreachable and if possible introducing an unreachable node at the end. However, of course if the erroring statement is the fallthrough terminator itself, there is no space for an unreachable node. We fix this by tweaking the verification to allow this case, as its really no worse than the other problems with fall-through terminators (#41476), but of course it would be good to address that as part of a more general IR refactor.
Keno
added a commit
that referenced
this pull request
May 13, 2023
This is yet another followup to #49692 and #49750. With the introduced change, we kill the CFG edge from the basic block with the discovered error to its successors. However, we have an invariant in the verifier that the CFG should always match the IR. Turns out this is for good reason, as we assume in a number of places (including, ironically in the irinterp) that a GotoNode/GotoIfNot terminator means that the BB has the corresponding number of successors in the IR. Fix all this by killing the rest of the basic block when we discover that it is unreachable and if possible introducing an unreachable node at the end. However, of course if the erroring statement is the fallthrough terminator itself, there is no space for an unreachable node. We fix this by tweaking the verification to allow this case, as its really no worse than the other problems with fall-through terminators (#41476), but of course it would be good to address that as part of a more general IR refactor.
Keno
added a commit
that referenced
this pull request
May 14, 2023
If a fall-through terminator was already Bottom, we should not attempt to rekill the successor edge, because it was already deleted. Yet another fix in the #49692, #49750, #49797 series, which is turning out to be quite a rabit hole. Also fix a typo in the verifer tweak where we were looking at the BB idx rather than the terminator idx.
Keno
added a commit
that referenced
this pull request
May 15, 2023
If a fall-through terminator was already Bottom, we should not attempt to rekill the successor edge, because it was already deleted. Yet another fix in the #49692, #49750, #49797 series, which is turning out to be quite a rabit hole. Also fix a typo in the verifer tweak where we were looking at the BB idx rather than the terminator idx.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I moved around some code in #49692 that broadened the replacement of statements by their const results. This is fine for how we're currently using irinterp in base, because we're requiring some fairly strong effects, but some downstream pipelines (and potentially Base in the future) want to use irinterp on code with arbitrary effects, so put in an appropriate check.