Skip to content

Commit c1c788e

Browse files
authored
print: fix intra-func (e.g. values) numbering to use print order instead of breadth-first. (#15)
This is a bit of an abstract problem, but hopefully this comparison shows the problem: <table> <tr><th>Before</th><th>After</th></tr> <tr> <td> ```rs // Note the "leaps" (e.g. v2 -> v5), // as the indentation level increases. v1 = … v2 = … if v2 { v5 = … if v5 { v9 = … v10 = … } v6 = … } v3 = … if v3 { v7 = … v8 = … } v4 = … // ^ last top-level is v4 so the // next level in will start at v5 ``` </td> <td> ```rs // The value numbering should // now match the textual order. v1 = … v2 = … if v2 { v3 = … if v3 { v4 = … v5 = … } v6 = … } v7 = … if v7 { v8 = … v9 = … } v10 = … // ^ last top-level (v10) is // now the last one overall ``` </td> </tr> </table> While the discrepancy isn't huge on this small artificial example, in practice the leap could be from e.g. 3-digit to 4-digit numbers, which wasn't just jarring but also made the dataflow hard to follow. This entire time I just assumed I was looking at values defined by later passes, but it turned out to be the result of region-level traversal (instead of handling all regions and nodes in regular `visit` order), and I only spotted it while tweaking this code, for a later change involving debuginfo (inlined call sites etc.).
2 parents 7767550 + 8f96858 commit c1c788e

File tree

1 file changed

+31
-22
lines changed

1 file changed

+31
-22
lines changed

src/print/mod.rs

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -938,24 +938,29 @@ impl<'a> Printer<'a> {
938938
def.deduped_attrs_across_versions.insert(attrs);
939939
}
940940
};
941-
let visit_region = |func_at_region: FuncAt<'_, Region>| {
942-
let region = func_at_region.position;
943-
944-
define(Use::AlignmentAnchorForRegion(region), None);
945-
// FIXME(eddyb) should labels have names?
946-
define(Use::RegionLabel(region), None);
947-
948-
let RegionDef { inputs, children, outputs: _ } = func_def_body.at(region).def();
949-
950-
for (i, input_decl) in inputs.iter().enumerate() {
951-
define(
952-
Use::RegionInput { region, input_idx: i.try_into().unwrap() },
953-
Some(input_decl.attrs),
954-
);
955-
}
956-
957-
for func_at_node in func_def_body.at(*children) {
958-
let node = func_at_node.position;
941+
// HACK(eddyb) this is as bad as it is due to the combination of:
942+
// - borrowing constraints on `define` (mutable access to maps)
943+
// - needing to minimize the changes to allow rebasing further
944+
// refactors (after which it may be easier to clean up anyway)
945+
let visit_region_or_node = |func_at_r_or_n: FuncAt<'_, Either<Region, Node>>| {
946+
let region_or_node = func_at_r_or_n.position;
947+
if let Either::Left(region) = region_or_node {
948+
define(Use::AlignmentAnchorForRegion(region), None);
949+
// FIXME(eddyb) should labels have names?
950+
define(Use::RegionLabel(region), None);
951+
952+
let RegionDef { inputs, children: _, outputs: _ } =
953+
func_def_body.at(region).def();
954+
955+
for (i, input_decl) in inputs.iter().enumerate() {
956+
define(
957+
Use::RegionInput { region, input_idx: i.try_into().unwrap() },
958+
Some(input_decl.attrs),
959+
);
960+
}
961+
} else {
962+
let node = region_or_node.right().unwrap();
963+
let func_at_node = func_at_r_or_n.at(node);
959964

960965
define(Use::AlignmentAnchorForNode(node), None);
961966

@@ -987,8 +992,8 @@ impl<'a> Printer<'a> {
987992
};
988993

989994
// FIXME(eddyb) maybe this should be provided by `visit`.
990-
struct VisitAllRegions<F>(F);
991-
impl<'a, F: FnMut(FuncAt<'a, Region>)> Visitor<'a> for VisitAllRegions<F> {
995+
struct VisitAllRegionsAndNodes<F>(F);
996+
impl<'a, F: FnMut(FuncAt<'a, Either<Region, Node>>)> Visitor<'a> for VisitAllRegionsAndNodes<F> {
992997
// FIXME(eddyb) this is excessive, maybe different kinds of
993998
// visitors should exist for module-level and func-level?
994999
fn visit_attr_set_use(&mut self, _: AttrSet) {}
@@ -998,11 +1003,15 @@ impl<'a> Printer<'a> {
9981003
fn visit_func_use(&mut self, _: Func) {}
9991004

10001005
fn visit_region_def(&mut self, func_at_region: FuncAt<'a, Region>) {
1001-
self.0(func_at_region);
1006+
self.0(func_at_region.at(Either::Left(func_at_region.position)));
10021007
func_at_region.inner_visit_with(self);
10031008
}
1009+
fn visit_node_def(&mut self, func_at_node: FuncAt<'a, Node>) {
1010+
self.0(func_at_node.at(Either::Right(func_at_node.position)));
1011+
func_at_node.inner_visit_with(self);
1012+
}
10041013
}
1005-
func_def_body.inner_visit_with(&mut VisitAllRegions(visit_region));
1014+
func_def_body.inner_visit_with(&mut VisitAllRegionsAndNodes(visit_region_or_node));
10061015
}
10071016

10081017
let mut region_label_counter = 0;

0 commit comments

Comments
 (0)