Document current relooper algorithm and do minor cleanup #1397
Conversation
randomPoison
changed the title
|
Ah, I see my error case that should never happen has happened. I'll look into that and likely just revert the |
| // Calculate the transitive closure of the successor map, i.e. the full set of | ||
| // labels reachable from each block, not including itself. Then flip the edges | ||
| // to get a map from a label to the set of labels that can reach it. | ||
| let strict_reachable_from = flip_edges(transitive_closure(&successor_map)); | ||
|
|
||
| // Flip the edges of the successor map to get a map of immediate predecessors | ||
| // for each block. |
There was a problem hiding this comment.
These comments don't seem to add information beyond exactly what the code is doing, so I'd remove them in favor of concision.
| // Try to match an existing branch point (from the initial C). See `MultipleInfo` for more | ||
| // information on this. | ||
| // Try to match an existing branch point (from the initial C). | ||
| // |
There was a problem hiding this comment.
Remove empty comment line..
|
|
||
| // Try to match an existing branch point (from the initial C). See `MultipleInfo` for more | ||
| // information on this. | ||
| // Try to match an existing branch point (from the initial C). |
There was a problem hiding this comment.
Leave the reference to MultipleInfo, but fix the existing /// TODO: document me in its documentation.
| // Try to match an existing branch point (from the initial C). | ||
| // | ||
| // We do this before creating a loop to better handle the cases where we have | ||
| // multiple entries that are part of disjoint loops. The loop analysis that |
There was a problem hiding this comment.
What do you mean by "disjoint" here? In the comment at the top of multiples.rs, it shows two CFG edges (the not-taken side of an if and the exit of a while loop) that coincide. But these two constructs are nested, not disjoint.
| recognized_c_multiple = true; | ||
|
|
||
| for (entry, content) in arms { | ||
| // Search through the arms of the C multiple to verify that our current CFG matches |
There was a problem hiding this comment.
Calling this a "C multiple" is confusing. Most of the code just calls this a Multiple, even though this information does come from C.
| // Check that we've only visited the expected nodes. If we've visited any nodes | ||
| // not in `content`, then the CFG differs from the C multiple and the search | ||
| // fails. |
There was a problem hiding this comment.
The logic here does seem to do what you describe, not what the comment said before. Should it be doing what it said before (ensuring that all of content is in visited rather than vice-versa), though? Intuitively I thought we wanted to find an exact match rather than a (possibly proper) subset.
The opposite direction, ensuring that nodes we visit are always in content, seems to be covered by the if !content.contains(&lbl) { check a few lines earlier.
2 participants
I've gone through the current relooper algorithm and added documentation and comments to clarify what the algorithm is currently doing. The module docs now have an overview of what the algorithm does, and I've added many verbose comments to help understand what each step of the algorithm is doing and why we do the things we do. I've aired on the side of verbosity for many of these comments, trying to capture all of the information I needed to understand the algorithm.
I've also done a small number of cleanup changes in places where the code was doing something unnecessary or was expressed in a way that I found hard to follow.
None of the changes should change behavior, expect for the one place where I added apanic!to surface an error case that was previously being silently ignored.Note that this is not meant to be reviewed one commit at a time. Intermediate commits sometimes have incorrect comments that I fix in later commits.