Compute dominator tree using semi-NCA algorithm #9603

amartosch · 2024-11-13T20:19:01Z

Compute dominator tree using semi-NCA algorithm, described in Linear-Time Algorithms for Dominators and Related Problems. Loukas Georgiadis, Princeton University, November 2005.
The algorithm has quadratic worst-case complexity, but usually is much faster. The speedup is minor, but can be noticed on large functions, i.e. in this file.
Also add a differential fuzzing job for the new algorithm, with the old one as baseline. This addition is dubious, because it couldn't find anything overnight.
Described in #9481.

github-actions · 2024-11-13T21:45:10Z

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "cranelift", "fuzzing"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Amanieu · 2024-11-14T14:25:01Z

cranelift/codegen/src/dominator_tree.rs

@@ -32,7 +423,7 @@ struct DomNode {
 }

 /// The dominator tree for a single function.
-pub struct DominatorTree {
+pub struct SimpleDominatorTree {


This should be moved to a separate file to make the code cleaner. At the moment it looks messy with 2 separate algorithms in the same file.

Moved to a separate file

cfallin

Thanks very much for this implementation! Some thoughts mostly on comments below but nothing major; given that you've done a fuzzing-based verification of the new algorithm against the old, and seen some compile-time speedups, I'm otherwise confident in it.

cfallin · 2024-11-14T23:47:22Z

cranelift/codegen/src/dominator_tree.rs

+    label: u32,
+    semi: u32,
+    idom: u32,
+}


Can we put doc-comments on the fields above? label, semi and idom don't tell me much about what's going on.

cfallin · 2024-11-14T23:49:08Z

cranelift/codegen/src/dominator_tree.rs

+
+    /// Returns pre_number for the new node
+    fn push(&mut self, ancestor: u32, block: Block) -> u32 {
+        // Push the virtual root if not present


This would be a little cleaner if we pushed the virtual root at construction time (and updated clear to truncate to length 1 instead), I think?

cfallin · 2024-11-14T23:50:02Z

cranelift/codegen/src/dominator_tree.rs

+}
+
+/// Traversal event to compute both preorder spanning tree
+/// and postorder block list. Don't use `Dfs` from traversals.rs


The "don't use..." comment seems out of place here: did you mean to give a reason why we don't use it here? Or is it a reminder (for what context?) or...?

cfallin · 2024-11-14T23:50:46Z

cranelift/codegen/src/dominator_tree.rs

    idom: PackedOption<Block>,
+    /// 0 for unreachable blocks


I'd prefer full sentences in doc-comments here (or at least complete thoughts) -- maybe something like "Preorder traversal number, or 0 for unreachable blocks." ?

cfallin · 2024-11-14T23:51:05Z

cranelift/codegen/src/dominator_tree.rs

 }

-/// The dominator tree for a single function.
+/// The dominator tree for a single function,
+/// computed using Semi-NCA algorithm


slight nitpick, but end comments with .?

cfallin · 2024-11-14T23:51:56Z

cranelift/codegen/src/dominator_tree.rs


    valid: bool,
 }

-/// Methods for querying the dominator tree.
+/// Query methods


Any reason we changed the doc-comment from Methods for querying the dominator tree. to Query methods (saying the same thing but missing detail, and no period)?

Yes, this is a mistake. Fixed.

cfallin · 2024-11-15T00:00:58Z

cranelift/codegen/src/dominator_tree.rs

+                    let pre_number = self.stree.push(parent, block);
+                    node.pre_number = pre_number;
+
+                    // Use the same traversal heuristics as in traversals.rs


Rather than refer to another source file here (which may change or be removed in the future), can we say what the heuristics are, briefly?

cfallin · 2024-11-15T00:01:49Z

cranelift/codegen/src/dominator_tree.rs

+                    self.dfs_worklist.extend(
+                        func.block_successors(block)
+                            .rev()
+                            .filter(|successor| self.nodes[*successor].pre_number == 0)


The 0 sentinel keeps occurring and I think it'd be clearer if we had a name for it -- could you define const NOT_VISITED: u32 = 0; or similar in this module?

cfallin · 2024-11-15T00:02:26Z

cranelift/codegen/src/dominator_tree.rs

-            self.nodes[block] = DomNode {
-                idom: self.compute_idom(block, cfg).into(),
-                rpo_number: (rpo_idx as u32 + 3) * STRIDE,
+    /// Eval-compress


This name ("eval-compress") doesn't mean anything to the reader -- could you give a more descriptive doc-comment?

cfallin · 2024-11-15T00:07:30Z

cranelift/codegen/src/verifier/mod.rs

@@ -300,14 +300,14 @@ pub fn verify_context<'a, FOI: Into<FlagsOrIsa<'a>>>(
 struct Verifier<'a> {
    func: &'a Function,
    expected_cfg: ControlFlowGraph,
-    expected_domtree: DominatorTree,
+    expected_domtree: SimpleDominatorTree,


Is there any reason we have to use the old ("simple") implementation here? Or is the idea that when doing verification, we want to fall back to the more trusted algorithm? (Could we document this reasoning in a comment if so?)

Actually, thinking aloud a bit: I think I'm OK with moving everything over to the new algorithm, including the verifier; it should be enough to have the fuzz-target that checks the algorithm itself against the old algorithm. (If we don't trust that, then we shouldn't use the new algorithm anywhere!)

Yes, the idea was to compare this tree to the more trusted algorithm. Changed to the new.

amartosch added 2 commits November 13, 2024 20:55

Add dominator tree computed using semi-NCA algorithm.

6dd2bbf

Add dominator tree fuzz target

39a63f6

amartosch requested review from a team as code owners November 13, 2024 20:19

amartosch requested review from alexcrichton and cfallin and removed request for a team November 13, 2024 20:19

github-actions bot added cranelift Issues related to the Cranelift code generator fuzzing Issues related to our fuzzing infrastructure labels Nov 13, 2024

alexcrichton removed their request for review November 14, 2024 05:29

Amanieu reviewed Nov 14, 2024

View reviewed changes

Move previous version of dominator tree to a separate file

7a8c128

cfallin approved these changes Nov 15, 2024

View reviewed changes

amartosch added 3 commits November 15, 2024 23:26

Improve comments.

97f19c6

Use the new dominator tree in verifier.

645bc3c

Remove unused iterators module.

faab154

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute dominator tree using semi-NCA algorithm #9603

Compute dominator tree using semi-NCA algorithm #9603

amartosch commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024

Amanieu Nov 14, 2024

amartosch Nov 14, 2024

cfallin left a comment

cfallin Nov 14, 2024

amartosch Nov 15, 2024

cfallin Nov 14, 2024

cfallin Nov 14, 2024

amartosch Nov 15, 2024

cfallin Nov 14, 2024

amartosch Nov 15, 2024

cfallin Nov 14, 2024

cfallin Nov 14, 2024

amartosch Nov 15, 2024

cfallin Nov 15, 2024

amartosch Nov 15, 2024

cfallin Nov 15, 2024

cfallin Nov 15, 2024

cfallin Nov 15, 2024

amartosch Nov 15, 2024

Compute dominator tree using semi-NCA algorithm #9603

Are you sure you want to change the base?

Compute dominator tree using semi-NCA algorithm #9603

Conversation

amartosch commented Nov 13, 2024 • edited Loading

github-actions bot commented Nov 13, 2024

Subscribe to Label Action

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfallin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amartosch commented Nov 13, 2024 •

edited

Loading