Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cranelift: Use DominatorTreePreorder in more places #7954

Open
jameysharp opened this issue Feb 16, 2024 · 4 comments
Open

cranelift: Use DominatorTreePreorder in more places #7954

jameysharp opened this issue Feb 16, 2024 · 4 comments
Labels
cranelift:E-easy Issues suitable for newcomers to investigate, including Rust newcomers! cranelift:goal:compile-time Focus area: how fast Cranelift can compile or how much memory it uses. cranelift:mid-end clif-to-clif related passes, legalizations, etc... good first issue Issues that are good for new contributors to tackle!

Comments

@jameysharp
Copy link
Contributor

Feature

We have two interfaces in Cranelift for navigating dominator trees, both defined in the cranelift_codegen::dominator_tree module: DominatorTree and DominatorTreePreorder. But we weren't using the latter outside of tests after #3434 landed in 2021, until I switched the egraph pass over to using it in #7948 today.

  1. We should audit all uses of DominatorTree to see if they would be better served by using DominatorTreePreorder instead.
  2. If we end up using DominatorTreePreorder in more places than just the egraph pass, then we should compute it once and share it.
  3. If it turns out that we always need a DominatorTreePreorder when compiling any function, then we should fold the two types into one and always compute the preorder when we're computing the dominator tree.

Benefit

These two interfaces both provide a method named dominates which checks whether one basic block dominates another. However, DominatorTree does this in time proportional to the length of the path from one block to the other within the dominator tree. Thanks to a linear-time preprocessing step performed once, DominatorTreePreorder can answer this question in constant time.

So if we're using the DominatorTree::dominates method anywhere that's performance-critical, switching to DominatorTreePreorder could provide an asymptotic-complexity improvement.

On top of that, sharing a precomputed preorder across multiple uses saves time redoing the preprocessing step and also may allow us to reuse a heap allocation for the temporary storage used during that preprocessing step.

Implementation

To start with, search for all uses of DominatorTree::dominates. For each one, see if we can just replace it with DominatorTreePreorder::dominates.

This is easy if both arguments are Block IDs, but either one is currently also allowed to be an instruction ID (Inst) or a ProgramPoint. If we're relying on that feature somewhere, it's only slightly more complicated: If two instructions are in the same block then the earlier instruction dominates the later instruction; otherwise we can go back to the easy case and compare the blocks they're in to see if one block dominates the other.

If some instances of DominatorTree are only being used to call dominates, then removing that structure from those instances in favor of DominatorTreePreorder is the next step. However, some cases may also be using other methods such as cfg_postorder or idom, which are not available on DominatorTreePreorder.

Alternatives

We can always leave this alone, but I think it's a good source of small changes that may give us performance improvements during compilation.

@jameysharp jameysharp added good first issue Issues that are good for new contributors to tackle! cranelift:goal:compile-time Focus area: how fast Cranelift can compile or how much memory it uses. cranelift:E-easy Issues suitable for newcomers to investigate, including Rust newcomers! cranelift:mid-end clif-to-clif related passes, legalizations, etc... labels Feb 16, 2024
@MuhtasimTanmoy
Copy link

Would like to take this one

@jameysharp
Copy link
Contributor Author

Great, please do! If you have any questions, let us know. We're happy to help!

@badumbatish
Copy link

hi there! is this issue still on going?

@badumbatish
Copy link

i'll give this one a try if nobody's doing this

badumbatish added a commit to badumbatish/wasmtime that referenced this issue Sep 7, 2024
 Changes to be committed:
	modified:   cranelift/codegen/src/alias_analysis.rs: Use
        dominator tree
        modified:   cranelift/codegen/src/context.rs: Use dominator tree
	modified:   cranelift/codegen/src/dominator_tree.rs: Fix
        dominates and computes and modify logic of domination
badumbatish added a commit to badumbatish/wasmtime that referenced this issue Sep 22, 2024
 Changes to be committed:
	modified:   cranelift/codegen/src/alias_analysis.rs: Use
        dominator tree
        modified:   cranelift/codegen/src/context.rs: Use dominator tree
	modified:   cranelift/codegen/src/dominator_tree.rs: Fix
        dominates and computes and modify logic of domination
badumbatish added a commit to badumbatish/wasmtime that referenced this issue Sep 26, 2024
 Changes to be committed:
	modified:   cranelift/codegen/src/alias_analysis.rs: Use
        dominator tree
        modified:   cranelift/codegen/src/context.rs: Use dominator tree
	modified:   cranelift/codegen/src/dominator_tree.rs: Fix
        dominates and computes and modify logic of domination
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:E-easy Issues suitable for newcomers to investigate, including Rust newcomers! cranelift:goal:compile-time Focus area: how fast Cranelift can compile or how much memory it uses. cranelift:mid-end clif-to-clif related passes, legalizations, etc... good first issue Issues that are good for new contributors to tackle!
Projects
None yet
Development

No branches or pull requests

3 participants