Skip to content

[DFAJumpThreading] Prevent pass from using too much memory. #145482

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dybv-sc
Copy link
Contributor

@dybv-sc dybv-sc commented Jun 24, 2025

The limit 'dfa-max-num-paths' that is used to control number of enumerated paths was not checked against inside getPathsFromStateDefMap. It may lead to large memory consumption for complex enough switch statements.

The limit 'dfa-max-num-paths' that is used to control number of enumerated paths
was not checked against inside getPathsFromStateDefMap. It may lead to large memory consumption for
complex enough switch statements.
@llvmbot
Copy link
Member

llvmbot commented Jun 24, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Bushev Dmitry (dybv-sc)

Changes

The limit 'dfa-max-num-paths' that is used to control number of enumerated paths was not checked against inside getPathsFromStateDefMap. It may lead to large memory consumption for complex enough switch statements.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp (+8)
diff --git a/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp b/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
index 938aab5879044..ac7af712bcf12 100644
--- a/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
@@ -618,6 +618,8 @@ struct AllSwitchPaths {
 
     VisitedBlocks UniqueBlocks;
     for (auto *IncomingBB : Phi->blocks()) {
+      if(Res.size() >= MaxNumPaths)
+        break;
       if (!UniqueBlocks.insert(IncomingBB).second)
         continue;
       if (!SwitchOuterLoop->contains(IncomingBB))
@@ -657,6 +659,8 @@ struct AllSwitchPaths {
             getPathsFromStateDefMap(StateDef, IncomingPhi, VB);
         for (ThreadingPath &Path : PredPaths) {
           Path.push_back(PhiBB);
+          if(Res.size() >= MaxNumPaths)
+            break;
           Res.push_back(std::move(Path));
         }
         continue;
@@ -679,6 +683,10 @@ struct AllSwitchPaths {
           ThreadingPath NewPath(Path);
           NewPath.appendExcludingFirst(IPath);
           NewPath.push_back(PhiBB);
+          if(Res.size() >= MaxNumPaths) {
+            VB.erase(PhiBB);
+            return Res;
+	  }
           Res.push_back(NewPath);
         }
       }

Copy link

github-actions bot commented Jun 24, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@dybv-sc
Copy link
Contributor Author

dybv-sc commented Jun 24, 2025

Example of this pass using too much memory:

Compilation of function Perl_sv_vcatpvfn_flags of module sv.c from 600.perlbench_s benchmark with -mllvm --enable-dfa-jump-thread uses up to 16GB of heap. Almost all of this memory is allocated for threading paths. With the fix applied memory consumption shrinks down to less than ~500 MB.

@dybv-sc dybv-sc requested a review from UsmanNadeem June 24, 2025 09:47
@dybv-sc dybv-sc added the bug Indicates an unexpected problem or unintended behavior label Jun 25, 2025
@dybv-sc dybv-sc requested a review from nikic June 27, 2025 09:57
@nikic nikic requested a review from XChy June 29, 2025 20:10
@@ -679,6 +683,10 @@ struct AllSwitchPaths {
ThreadingPath NewPath(Path);
NewPath.appendExcludingFirst(IPath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IntermediatePaths are appended to the list of PredPaths (here, and also in the run() function), so the total number of paths is the multiplication of the sizes of the two lists. IntermediatePaths are already calculated so we are wasting compute cycles here by doing wasteful computation and by breaking the loop later than we should.

Since getPathsFromStateDefMap() and paths() are called multiple times and from different places they don't see the global picture of the number of paths we already have.

My suggestion is to add an argument to these two functions and update every call to these functions with a more appropriate limit instead of MaxNumPaths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior llvm:transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants