Skip to content

Conversation

@kazutakahirata
Copy link
Contributor

This patch changes the type of VisitedMap to DenseSet from DenseMap
because the value side of the map is always "true".

Technically:

if (VisitedMap[*SI])

inserts "false" as a value, but the value is immediately overridden
with:

VisitedMap[*SI] = true;

While we are at it, this patch removes the repeated hash lookups
around the "if" statement.

This patch changes the type of VisitedMap to DenseSet from DenseMap
because the value side of the map is always "true".

Technically:

  if (VisitedMap[*SI])

inserts "false" as a value, but the value is immediately overridden
with:

  VisitedMap[*SI] = true;

While we are at it, this patch removes the repeated hash lookups
around the "if" statement.
@llvmbot
Copy link
Member

llvmbot commented Feb 14, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Kazu Hirata (kazutakahirata)

Changes

This patch changes the type of VisitedMap to DenseSet from DenseMap
because the value side of the map is always "true".

Technically:

if (VisitedMap[*SI])

inserts "false" as a value, but the value is immediately overridden
with:

VisitedMap[*SI] = true;

While we are at it, this patch removes the repeated hash lookups
around the "if" statement.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/PartialInlining.cpp (+4-5)
diff --git a/llvm/lib/Transforms/IPO/PartialInlining.cpp b/llvm/lib/Transforms/IPO/PartialInlining.cpp
index cead7b84c3fc8..ea75b637c1646 100644
--- a/llvm/lib/Transforms/IPO/PartialInlining.cpp
+++ b/llvm/lib/Transforms/IPO/PartialInlining.cpp
@@ -412,9 +412,9 @@ PartialInlinerImpl::computeOutliningColdRegionsInfo(
   bool ColdCandidateFound = false;
   BasicBlock *CurrEntry = EntryBlock;
   std::vector<BasicBlock *> DFS;
-  DenseMap<BasicBlock *, bool> VisitedMap;
+  DenseSet<BasicBlock *> VisitedSet;
   DFS.push_back(CurrEntry);
-  VisitedMap[CurrEntry] = true;
+  VisitedSet.insert(CurrEntry);
 
   // Use Depth First Search on the basic blocks to find CFG edges that are
   // considered cold.
@@ -432,9 +432,8 @@ PartialInlinerImpl::computeOutliningColdRegionsInfo(
         BBProfileCount(ThisBB) < MinBlockCounterExecution)
       continue;
     for (auto SI = succ_begin(ThisBB); SI != succ_end(ThisBB); ++SI) {
-      if (VisitedMap[*SI])
+      if (!VisitedSet.insert(*SI).second)
         continue;
-      VisitedMap[*SI] = true;
       DFS.push_back(*SI);
       // If branch isn't cold, we skip to the next one.
       BranchProbability SuccProb = BPI.getEdgeProbability(ThisBB, *SI);
@@ -492,7 +491,7 @@ PartialInlinerImpl::computeOutliningColdRegionsInfo(
       // at inner regions because the outer region may have live-exit
       // variables.
       for (auto *BB : DominateVector)
-        VisitedMap[BB] = true;
+        VisitedSet.insert(BB);
 
       // ReturnBlock here means the block after the outline call
       BasicBlock *ReturnBlock = ExitBlock->getSingleSuccessor();

@kazutakahirata kazutakahirata merged commit d3d2ea6 into llvm:main Feb 14, 2025
8 checks passed
@kazutakahirata kazutakahirata deleted the cleanup_001_repeated_hash_lookups_llvm_PartialInlining branch February 14, 2025 10:33
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Feb 14, 2025
This patch changes the type of VisitedMap to DenseSet from DenseMap
because the value side of the map is always "true".

Technically:

  if (VisitedMap[*SI])

inserts "false" as a value, but the value is immediately overridden
with:

  VisitedMap[*SI] = true;

While we are at it, this patch removes the repeated hash lookups
around the "if" statement.
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Feb 24, 2025
This patch changes the type of VisitedMap to DenseSet from DenseMap
because the value side of the map is always "true".

Technically:

  if (VisitedMap[*SI])

inserts "false" as a value, but the value is immediately overridden
with:

  VisitedMap[*SI] = true;

While we are at it, this patch removes the repeated hash lookups
around the "if" statement.
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