Skip to content

Conversation

@Keno
Copy link
Member

@Keno Keno commented Jan 11, 2024

When IncremetnalCompact deletes and edge between blocks, it needs to go to the target and delete any reference to the edge from the PhiNodes therein. What happened in #52858, is that we had a situtation like:

# Block n-1
%a Expr(...)
Expr(...) # <- This node is pending at the start of compaction
# Block n
%b PhiNode

To do the deletion, we use CompactPeekIterator, which gets a list of original statements and iterates over all the statements inside the range, including pending ones. However, there is a semantic confusion about whether to include all all statements since the previous statement (%a above), or only those statements that are actually attached to the first instruction (%b above). This of course doesn't really matter usually unless you're at a BasicBlock boundary.

For the present case, we really do not want the instructions in the previous basic block - the iteration of PhiNodes terminates if it seems a stmt that cannot be in the phi block, so mistakenly seeing one of those instructions causes it to fail to fix a PhiNode that it should have, causing #52858.

We fix #52858 by giving the CompactPeekIterator a flag to only return stmts in the correct basic block. While we're at it, also fix the condition for what kinds of statements are allowed in the phi block, as that was clarified more recently than this code was written.

When IncremetnalCompact deletes and edge between blocks, it needs
to go to the target and delete any reference to the edge from
the PhiNodes therein. What happened in #52858, is that we had
a situtation like:

```
# Block n-1
%a Expr(...)
Expr(...) # <- This node is pending at the start of compaction
# Block n
%b PhiNode
```

To do the deletion, we use `CompactPeekIterator`, which gets
a list of original statements and iterates over all the statements
inside the range, including pending ones. However, there is a
semantic confusion about whether to include all all statements
since the previous statement (%a above), or only those statements
that are actually attached to the first instruction (%b above).
This of course doesn't really matter usually unless you're at
a BasicBlock boundary.

For the present case, we really do not want the instructions in
the previous basic block - the iteration of PhiNodes terminates
if it seems a stmt that cannot be in the phi block, so mistakenly
seeing one of those instructions causes it to fail to fix a PhiNode
that it should have, causing #52858.

We fix #52858 by giving the CompactPeekIterator a flag to only
return stmts in the correct basic block. While we're at it, also
fix the condition for what kinds of statements are allowed in the
phi block, as that was clarified more recently than this code
was written.
@oscardssmith oscardssmith added the bugfix This change fixes an existing bug label Jan 11, 2024
@Keno Keno merged commit 314d40f into master Jan 12, 2024
@Keno Keno deleted the kf/52858 branch January 12, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This change fixes an existing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IR verification error during testing of UnicodePlots.jl

2 participants