Skip to content

[DomTree] Provide a way to query if DFSInfo is valid #91251

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

Closed
wants to merge 1 commit into from

Conversation

hiraditya
Copy link
Collaborator

Pass like GVNSink modify CFG use DFS numbers for storing basic blocks (See #90995).
Instead of updating DFS numbers after every modification to CFG,
it is more efficient to do lazily i.e., only when DFS numbers are going to be used by the pass.

Pass like GVNSink modify CFG use DFS numbers for storing basic blocks.
Instead of updating DFS numbers after every modification to CFG,
it is more efficient to do lazily i.e., only when DFS numbers are
going to be used by the pass.
@llvmbot
Copy link
Member

llvmbot commented May 6, 2024

@llvm/pr-subscribers-llvm-support

Author: AdityaK (hiraditya)

Changes

Pass like GVNSink modify CFG use DFS numbers for storing basic blocks (See #90995).
Instead of updating DFS numbers after every modification to CFG,
it is more efficient to do lazily i.e., only when DFS numbers are going to be used by the pass.


Full diff: https://github.com/llvm/llvm-project/pull/91251.diff

1 Files Affected:

  • (modified) llvm/include/llvm/Support/GenericDomTree.h (+2)
diff --git a/llvm/include/llvm/Support/GenericDomTree.h b/llvm/include/llvm/Support/GenericDomTree.h
index 4fce9d8bfb2b68..91dd58f1b60873 100644
--- a/llvm/include/llvm/Support/GenericDomTree.h
+++ b/llvm/include/llvm/Support/GenericDomTree.h
@@ -328,6 +328,8 @@ class DominatorTreeBase {
   ///
   bool isPostDominator() const { return IsPostDominator; }
 
+  bool isDFSInfoValid() const { return DFSInfoValid; }
+
   /// compare - Return false if the other dominator tree base matches this
   /// dominator tree base. Otherwise return true.
   bool compare(const DominatorTreeBase &Other) const {

@efriedma-quic
Copy link
Collaborator

I don't see why you'd need this; updateDFSNumbers short-circuits if the numbers are already up-to-date.

@hiraditya
Copy link
Collaborator Author

I don't see why you'd need this; updateDFSNumbers short-circuits if the numbers are already up-to-date.

Let's say i want to use DFSIn number, how do i know DFSIn number is valid calling updateDFSNumbers?

The use case is:

  • in the beginning of pass i call updateDFSNumbers
  • i use dfsin numbers for ordering
  • modify (or not) CFG to split basic blocks
  • i want to use dfs in numbers.

One approach would be to keep a boolean (CFGModified for example) in the pass itself. but having this API will simplify it.

@efriedma-quic
Copy link
Collaborator

how do i know DFSIn number is valid calling updateDFSNumbers?

if (!DT.isDFSInfoValid()) DT.updateDFSNumbers() is exactly the same as DT.updateDFSNumbers(). So I can't see why it would matter... unless you need to do some other bookkeeping to discard the invalid DFS numbers?

And as I noted on the other review, calling DT.updateDFSNumbers() after processing every basic block has extremely bad performance characteristics in edge cases.

@hiraditya
Copy link
Collaborator Author

how do i know DFSIn number is valid calling updateDFSNumbers?

if (!DT.isDFSInfoValid()) DT.updateDFSNumbers() is exactly the same as DT.updateDFSNumbers(). So I can't see why it would matter... unless you need to do some other bookkeeping to discard the invalid DFS numbers?

And as I noted on the other review, calling DT.updateDFSNumbers() after processing every basic block has extremely bad performance characteristics in edge cases.

makes sense. i should be able make it work without this API.

@hiraditya hiraditya closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants