print: align adjacent multi-version columns' lines, to match up their…

… anchors. (#18) This extra step, just before turning multi-version pretty-printed fragments into a HTML table row, allows for a clearer visual match between the columns, turning the multi-version mode into something closer to a pass "differ", but the use of *anchors* (instead of line contents) makes it more conservative, reliable and somewhat simpler. In short, if two adjacent versions contain a *definition* like `v123 = ...`, it should now end up on the same horizontal line in both versions (assuming it's not e.g. out of order wrt most of the other definitions). Before, adding or removing instructions (or even their pretty-printing needing different line amounts) would result in one side being behind (and the other, ahead), which would accumulate (even if most of their contents might be similar or even identical). With this PR, a lot of that should be alleviated, as padding (of empty lines) is added to keep both versions aligned as much as possible. --- *However*, while it works great for correcting *small* misalignments (the original usecase), it's quite zealous (and lacks any heuristics or further constraint-solving etc.), leading to results like this: |[**Before**](https://htmlpreview.github.io/?https://gist.github.com/eddyb/387fb2b7e128e8ca4e4c00ff82baf459/raw/0-before-spirt%252318-simplest_shader.spirt.html&dark#func1)<br><sub><sup>(click for complete<br>pretty HTML example)</sup></sub>|![image](https://user-images.githubusercontent.com/77424/224555823-318a9019-3463-4f9a-ac61-430d48197b72.png)| |:-:|-| |[**After**](https://htmlpreview.github.io/?https://gist.github.com/eddyb/387fb2b7e128e8ca4e4c00ff82baf459/raw/1-after-spirt%252318-simplest_shader.spirt.html&dark#func1)<br><sub><sup>(click for complete<br>pretty HTML example)</sup></sub>|![image](https://user-images.githubusercontent.com/77424/229123954-fd79df73-0cd4-41c6-b515-59646c0bdc5e.png)| This is for the same SPIR-T as the last screenshot from [the Rust-GPU `reduce`+`fuse_selects` PR](EmbarkStudios/rust-gpu#988 (comment)). The `OpNop`s don't help (and those should be gone once SPIR-T APIs are adjusted), but it's generally much worse looking than I was hoping. Maybe it's worth it for the ability, but some things are just annoying. Based on this screenshot alone, several possible further improvements come to mind: <sub>(_**EDIT**: several of these have been implemented in this PR since it was opened_)</sub> * [x] ~~hidden anchors could be added for e.g. `ControlNode`s and `ControlRegion`s, even if there's not necessarily any syntax to attach them to (other than the keyword for non-`Block` `ControlNode`s and the `{...}` brackets for `ControlRegion`?)~~ * [x] ~~the contents of the aligned lines could be compared and some kind of graying/translucency used to de-emphasize the lines that haven't changed~~ * ~~there's a risk here of making them confused with *removed* lines~~ * [x] ~~this is where merely *partial* grayscaling, or some kind of sepia effect, might come in handy~~ * [x] ~~such diff-like mode could supplant the use of `colspan` *entirely*, and allow horizontal scrolling of the whole table, with the max line length being used to size the cells (e.g. `td { max-width: 100ch; }`), instead of divvying up the whole viewport width~~ * <sub>may need some level of interactivity (based on which column is being hovered on?)</sub> * Rust-GPU's `--spirt-dump-passes` could skip the unstructured control-flow column, which adds a bunch of gaps for no good reason (or it could be kept but with the alignment turned off?) * <sub>euristics (or just more user control) to avoid large gaps</sub> * <sub>some additional "layout" pass could rearrange all the lines that are "floating" in between anchors</sub> * <sub>crucially, the current state is that the new vertical gaps are _always_ inserted just before the anchor, _and never elsewhere_, but the lines before the anchor might be more "attached" to it than the ones after (sadly, we can't just switch where the vertical gap goes unconditionally: anchors can easily precede multiple-line contents that _should not_ be broken up)</sub> * <sub>e.g. ideally the vertical gap would be at the outermost indentation level, which would allow e.g. structured control-flow to "coalesce together" (may require either using `LineOp` more directly, or just making indentation special in `TextOp`)</sub> * <sub>it almost feels like this requires a redesign of a bunch of the `print::pretty` layout, to allow "flexible vertical spacing", which frankly seems like overkill</sub> * <sub>dynamic control over the HTML table via JS, to allow user adjustments on the fly</sub> * <sub>some part of the anchor alignment algorithm would likely need to be implemented in JS, to avoid the combinatorial explosion in the data that would need to be statically generated otherwise</sub>
EmbarkStudios · Mar 31, 2023 · e47d428 · e47d428
2 parents 2f47d75 + eaf7476
commit e47d428
Show file tree

Hide file tree

Showing 6 changed files with 1,145 additions and 443 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -33,6 +33,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased] - ReleaseDate
 
+### Added ⭐
+- [PR#18](https://github.com/EmbarkStudios/spirt/pull/18) added anchor-based alignment
+  to multi-version pretty-printing output (the same definitions will be kept on
+  the same lines in all columns, wherever possible, to improve readability)
+
 ### Changed 🛠
 - [PR#26](https://github.com/EmbarkStudios/spirt/pull/26) allowed using `OpEmitMeshTasksEXT` as a terminator (by hardcoding it as `Control-Flow`)
 - [PR#25](https://github.com/EmbarkStudios/spirt/pull/25) updated SPIRV-headers from 1.5.4 to 1.6.1

diff --git a/Cargo.toml b/Cargo.toml
@@ -24,8 +24,10 @@ arrayvec = "0.7.1"
 bytemuck = "1.12.3"
 elsa = { version = "1.6.0", features = ["indexmap"] }
 indexmap = "1.7.0"
+internal-iterator = "0.2.0"
 itertools = "0.10.3"
 lazy_static = "1.4.0"
+longest-increasing-subsequence = "0.1.0"
 rustc-hash = "1.1.0"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"

diff --git a/src/cfg.rs b/src/cfg.rs
@@ -78,16 +78,20 @@ impl ControlFlowGraph {
         func_def_body: &FuncDefBody,
     ) -> impl DoubleEndedIterator<Item = ControlRegion> {
         let mut post_order = SmallVec::<[_; 8]>::new();
-        {
-            let mut incoming_edge_counts = EntityOrientedDenseMap::new();
-            self.traverse_whole_func(
-                func_def_body,
-                &mut incoming_edge_counts,
-                &mut |_| {},
-                &mut |region| post_order.push(region),
-            );
-        }
+        self.traverse_whole_func(
+            func_def_body,
+            &mut TraversalState {
+                incoming_edge_counts: EntityOrientedDenseMap::new(),
+
+                pre_order_visit: |_| {},
+                post_order_visit: |region| post_order.push(region),
 
+                // NOTE(eddyb) this doesn't impact semantics, but combined with
+                // the final reversal, it should keep targets in the original
+                // order in the cases when they didn't get deduplicated.
+                reverse_targets: true,
+            },
+        );
         post_order.into_iter().rev()
     }
 }
@@ -120,15 +124,23 @@ mod sealed {
         }
     }
 }
+use itertools::Either;
 use sealed::IncomingEdgeCount;
 
+struct TraversalState<PreVisit: FnMut(ControlRegion), PostVisit: FnMut(ControlRegion)> {
+    incoming_edge_counts: EntityOrientedDenseMap<ControlRegion, IncomingEdgeCount>,
+    pre_order_visit: PreVisit,
+    post_order_visit: PostVisit,
+
+    // FIXME(eddyb) should this be a generic parameter for "targets iterator"?
+    reverse_targets: bool,
+}
+
 impl ControlFlowGraph {
     fn traverse_whole_func(
         &self,
         func_def_body: &FuncDefBody,
-        incoming_edge_counts: &mut EntityOrientedDenseMap<ControlRegion, IncomingEdgeCount>,
-        pre_order_visit: &mut impl FnMut(ControlRegion),
-        post_order_visit: &mut impl FnMut(ControlRegion),
+        state: &mut TraversalState<impl FnMut(ControlRegion), impl FnMut(ControlRegion)>,
     ) {
         let func_at_body = func_def_body.at_body();
 
@@ -139,47 +151,43 @@ impl ControlFlowGraph {
         ));
         assert!(func_at_body.def().outputs.is_empty());
 
-        self.traverse(
-            func_at_body,
-            incoming_edge_counts,
-            pre_order_visit,
-            post_order_visit,
-        );
+        self.traverse(func_at_body, state);
     }
 
     fn traverse(
         &self,
         func_at_region: FuncAt<'_, ControlRegion>,
-        incoming_edge_counts: &mut EntityOrientedDenseMap<ControlRegion, IncomingEdgeCount>,
-        pre_order_visit: &mut impl FnMut(ControlRegion),
-        post_order_visit: &mut impl FnMut(ControlRegion),
+        state: &mut TraversalState<impl FnMut(ControlRegion), impl FnMut(ControlRegion)>,
     ) {
         let region = func_at_region.position;
 
         // FIXME(eddyb) `EntityOrientedDenseMap` should have an `entry` API.
-        if let Some(existing_count) = incoming_edge_counts.get_mut(region) {
+        if let Some(existing_count) = state.incoming_edge_counts.get_mut(region) {
             *existing_count += IncomingEdgeCount::ONE;
             return;
         }
-        incoming_edge_counts.insert(region, IncomingEdgeCount::ONE);
+        state
+            .incoming_edge_counts
+            .insert(region, IncomingEdgeCount::ONE);
 
-        pre_order_visit(region);
+        (state.pre_order_visit)(region);
 
         let control_inst = self
             .control_inst_on_exit_from
             .get(region)
             .expect("cfg: missing `ControlInst`, despite having left structured control-flow");
 
-        for &target in &control_inst.targets {
-            self.traverse(
-                func_at_region.at(target),
-                incoming_edge_counts,
-                pre_order_visit,
-                post_order_visit,
-            );
+        let targets = control_inst.targets.iter().copied();
+        let targets = if state.reverse_targets {
+            Either::Left(targets.rev())
+        } else {
+            Either::Right(targets)
+        };
+        for target in targets {
+            self.traverse(func_at_region.at(target), state);
         }
 
-        post_order_visit(region);
+        (state.post_order_visit)(region);
     }
 }
 
@@ -381,15 +389,21 @@ impl<'a> Structurizer<'a> {
             ctor_args: [].into_iter().collect(),
         });
 
-        let mut incoming_edge_counts = EntityOrientedDenseMap::new();
-        if let Some(cfg) = &func_def_body.unstructured_cfg {
-            cfg.traverse_whole_func(
-                func_def_body,
-                &mut incoming_edge_counts,
-                &mut |_| {},
-                &mut |_| {},
-            );
-        }
+        let incoming_edge_counts = func_def_body
+            .unstructured_cfg
+            .as_ref()
+            .map(|cfg| {
+                let mut state = TraversalState {
+                    incoming_edge_counts: EntityOrientedDenseMap::new(),
+
+                    pre_order_visit: |_| {},
+                    post_order_visit: |_| {},
+                    reverse_targets: false,
+                };
+                cfg.traverse_whole_func(func_def_body, &mut state);
+                state.incoming_edge_counts
+            })
+            .unwrap_or_default();
 
         Self {
             cx,