Skip to content

[BOLT] Eliminate dead jump tables #91666

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

Conversation

aaupov
Copy link
Contributor

@aaupov aaupov commented May 9, 2024

Dead jump tables such as those arising from FIXED_PIC_BRANCH
optimization don't need to be updated or moved. Further, if any jump
table entry points to a block removed by unreachable code elimination,
we would be unable to update it and jump table emission would fail.

Identify non-referenced jump tables and delete them to prevent that.

Test Plan: ninja check-bolt

Created using spr 1.3.4
@llvmbot
Copy link
Member

llvmbot commented May 9, 2024

@llvm/pr-subscribers-bolt

Author: Amir Ayupov (aaupov)

Changes

Dead jump tables such as those arising from FIXED_PIC_BRANCH
optimization don't need to be updated or moved. Further, if any jump
table entry points to a block removed by unreachable code elimination,
we would be unable to update it and jump table emission would fail.

Identify non-referenced jump tables and delete them to prevent that.

Test Plan: NFC for existing tests.


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

1 Files Affected:

  • (modified) bolt/lib/Core/BinaryFunction.cpp (+20)
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 11103f7bdce8..f74ecea8ac0a 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1697,6 +1697,26 @@ void BinaryFunction::postProcessEntryPoints() {
 }
 
 void BinaryFunction::postProcessJumpTables() {
+  // Set of JTs accessed from this function.
+  std::unordered_set<uint64_t> LiveJTs;
+  for (auto &JTSite : JTSites)
+    LiveJTs.emplace(JTSite.second);
+
+  // Remove dead jump tables (reference removed as a result of
+  // POSSIBLE_PIC_FIXED_BRANCH optimization).
+  for (auto JTI = JumpTables.begin(), JTE = JumpTables.end(); JTI != JTE; ) {
+    const uint64_t Address = JTI->first;
+    JumpTable *JT = JTI->second;
+    bool HasOneParent = JT->Parents.size() == 1;
+    if (LiveJTs.count(Address) == 0 && HasOneParent) {
+      BC.deregisterJumpTable(Address);
+      delete JT;
+      JTI = JumpTables.erase(JTI);
+      continue;
+    }
+    ++JTI;
+  }
+
   // Create labels for all entries.
   for (auto &JTI : JumpTables) {
     JumpTable &JT = *JTI.second;

aaupov added 3 commits May 9, 2024 16:31
Created using spr 1.3.4
Created using spr 1.3.4
Copy link
Contributor

@rafaelauler rafaelauler left a comment

Choose a reason for hiding this comment

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

Looks like this is dangerous and crashes binaries in our tests? Do you still want to move forward with this stack? Or are you working on it? If this stack is work in progress, maybe mark it as "changes planned" or the equivalent in github.

@aaupov aaupov marked this pull request as draft May 22, 2024 21:17
@aaupov
Copy link
Contributor Author

aaupov commented May 22, 2024

Looks like this is dangerous and crashes binaries in our tests? Do you still want to move forward with this stack? Or are you working on it? If this stack is work in progress, maybe mark it as "changes planned" or the equivalent in github.

Yes, we'll need to discuss how to move forward with it, as it's unsafe as is. Marking as draft for now.

@aaupov aaupov closed this Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants