Skip to content

[IR] Use single map in PredIteratorCache (NFC) #101950

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

Merged
merged 2 commits into from
Aug 5, 2024
Merged

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Aug 5, 2024

Use a single map storing ArrayRef instead of two separate maps for the base pointer and size. This saves one map lookup per cache accesses.

This is an alternative to #101885 with a similar compile-time improvement: http://llvm-compile-time-tracker.com/compare.php?from=1fa7f05b709748e8a36936cbb5508070c8214354&to=1af82f2ca51aa324c772941770dd3ee4043b2b4d&stat=instructions:u

Use a single map storing ArrayRef instead of two separate maps
for the base pointer and size. This saves one map lookup per
cache accesses.

This is an alternative to llvm#101885.
@nikic nikic requested a review from aengelke August 5, 2024 09:59
@llvmbot llvmbot added the llvm:ir label Aug 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 5, 2024

@llvm/pr-subscribers-llvm-ir

Author: Nikita Popov (nikic)

Changes

Use a single map storing ArrayRef instead of two separate maps for the base pointer and size. This saves one map lookup per cache accesses.

This is an alternative to #101885 with a similar compile-time improvement: http://llvm-compile-time-tracker.com/compare.php?from=1fa7f05b709748e8a36936cbb5508070c8214354&to=1af82f2ca51aa324c772941770dd3ee4043b2b4d&stat=instructions:u


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

1 Files Affected:

  • (modified) llvm/include/llvm/IR/PredIteratorCache.h (+10-33)
diff --git a/llvm/include/llvm/IR/PredIteratorCache.h b/llvm/include/llvm/IR/PredIteratorCache.h
index fc8cf20e9f759..d52e78b25fc33 100644
--- a/llvm/include/llvm/IR/PredIteratorCache.h
+++ b/llvm/include/llvm/IR/PredIteratorCache.h
@@ -25,52 +25,29 @@ namespace llvm {
 /// predecessor iterator queries.  This is useful for code that repeatedly
 /// wants the predecessor list for the same blocks.
 class PredIteratorCache {
-  /// BlockToPredsMap - Pointer to null-terminated list.
-  mutable DenseMap<BasicBlock *, BasicBlock **> BlockToPredsMap;
-  mutable DenseMap<BasicBlock *, unsigned> BlockToPredCountMap;
+  /// Cached list of predecessors, allocated in Memory.
+  mutable DenseMap<BasicBlock *, ArrayRef<BasicBlock *>> BlockToPredsMap;
 
   /// Memory - This is the space that holds cached preds.
   BumpPtrAllocator Memory;
 
-private:
-  /// GetPreds - Get a cached list for the null-terminated predecessor list of
-  /// the specified block.  This can be used in a loop like this:
-  ///   for (BasicBlock **PI = PredCache->GetPreds(BB); *PI; ++PI)
-  ///      use(*PI);
-  /// instead of:
-  /// for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI)
-  BasicBlock **GetPreds(BasicBlock *BB) {
-    BasicBlock **&Entry = BlockToPredsMap[BB];
-    if (Entry)
+public:
+  size_t size(BasicBlock *BB) { return get(BB).size(); }
+  ArrayRef<BasicBlock *> get(BasicBlock *BB) {
+    ArrayRef<BasicBlock *> &Entry = BlockToPredsMap[BB];
+    if (Entry.data())
       return Entry;
 
     SmallVector<BasicBlock *, 32> PredCache(predecessors(BB));
-    PredCache.push_back(nullptr); // null terminator.
-
-    BlockToPredCountMap[BB] = PredCache.size() - 1;
-
-    Entry = Memory.Allocate<BasicBlock *>(PredCache.size());
-    std::copy(PredCache.begin(), PredCache.end(), Entry);
+    BasicBlock **Data = Memory.Allocate<BasicBlock *>(PredCache.size());
+    std::copy(PredCache.begin(), PredCache.end(), Data);
+    Entry = ArrayRef(Data, PredCache.size());
     return Entry;
   }
 
-  unsigned GetNumPreds(BasicBlock *BB) const {
-    auto Result = BlockToPredCountMap.find(BB);
-    if (Result != BlockToPredCountMap.end())
-      return Result->second;
-    return BlockToPredCountMap[BB] = pred_size(BB);
-  }
-
-public:
-  size_t size(BasicBlock *BB) const { return GetNumPreds(BB); }
-  ArrayRef<BasicBlock *> get(BasicBlock *BB) {
-    return ArrayRef(GetPreds(BB), GetNumPreds(BB));
-  }
-
   /// clear - Remove all information.
   void clear() {
     BlockToPredsMap.clear();
-    BlockToPredCountMap.clear();
     Memory.Reset();
   }
 };

Copy link
Contributor

@aengelke aengelke left a comment

Choose a reason for hiding this comment

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

LGTM with nit

mutable DenseMap<BasicBlock *, BasicBlock **> BlockToPredsMap;
mutable DenseMap<BasicBlock *, unsigned> BlockToPredCountMap;
/// Cached list of predecessors, allocated in Memory.
mutable DenseMap<BasicBlock *, ArrayRef<BasicBlock *>> BlockToPredsMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

No mutable.

@nikic nikic merged commit 6676f79 into llvm:main Aug 5, 2024
7 checks passed
@nikic nikic deleted the pred-cache-single branch August 5, 2024 11:00
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