Skip to content

[Transforms][NFC] Tiny fixes in SplitModule #95903

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.

Sign up for GitHub

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
Jul 2, 2024
Merged

[Transforms][NFC] Tiny fixes in SplitModule #95903

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8456f2f
[Transforms][NFC] Tiny fixes in SplitModule
sergachev Jun 5, 2024
8ef43a8
Remove unnecessary parentheses
sergachev Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions llvm/lib/Transforms/Utils/SplitModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ static void findPartitions(Module &M, ClusterIDMapType &ClusterIDMap,
// At this point module should have the proper mix of globals and locals.
// As we attempt to partition this module, we must not change any
// locals to globals.
LLVM_DEBUG(dbgs() << "Partition module with (" << M.size() << ")functions\n");
LLVM_DEBUG(dbgs() << "Partition module with (" << M.size()
<< ") functions\n");
ClusterMapType GVtoClusterMap;
ComdatMembersType ComdatMembers;

Expand Down Expand Up @@ -164,10 +165,10 @@ static void findPartitions(Module &M, ClusterIDMapType &ClusterIDMap,
std::priority_queue<std::pair<unsigned, unsigned>,
std::vector<std::pair<unsigned, unsigned>>,
decltype(CompareClusters)>
BalancinQueue(CompareClusters);
BalancingQueue(CompareClusters);
// Pre-populate priority queue with N slot blanks.
for (unsigned i = 0; i < N; ++i)
BalancinQueue.push(std::make_pair(i, 0));
BalancingQueue.push(std::make_pair(i, 0));

using SortType = std::pair<unsigned, ClusterMapType::iterator>;

Expand All @@ -177,11 +178,13 @@ static void findPartitions(Module &M, ClusterIDMapType &ClusterIDMap,
// To guarantee determinism, we have to sort SCC according to size.
// When size is the same, use leader's name.
for (ClusterMapType::iterator I = GVtoClusterMap.begin(),
E = GVtoClusterMap.end(); I != E; ++I)
E = GVtoClusterMap.end();
I != E; ++I)
if (I->isLeader())
Sets.push_back(
std::make_pair(std::distance(GVtoClusterMap.member_begin(I),
GVtoClusterMap.member_end()), I));
GVtoClusterMap.member_end()),
I));

llvm::sort(Sets, [](const SortType &a, const SortType &b) {
if (a.first == b.first)
Expand All @@ -191,9 +194,9 @@ static void findPartitions(Module &M, ClusterIDMapType &ClusterIDMap,
});

for (auto &I : Sets) {
unsigned CurrentClusterID = BalancinQueue.top().first;
unsigned CurrentClusterSize = BalancinQueue.top().second;
BalancinQueue.pop();
unsigned CurrentClusterID = BalancingQueue.top().first;
unsigned CurrentClusterSize = BalancingQueue.top().second;
BalancingQueue.pop();

LLVM_DEBUG(dbgs() << "Root[" << CurrentClusterID << "] cluster_size("
<< I.first << ") ----> " << I.second->getData()->getName()
Expand All @@ -211,7 +214,7 @@ static void findPartitions(Module &M, ClusterIDMapType &ClusterIDMap,
CurrentClusterSize++;
}
// Add this set size to the number of entries in this cluster.
BalancinQueue.push(std::make_pair(CurrentClusterID, CurrentClusterSize));
BalancingQueue.push(std::make_pair(CurrentClusterID, CurrentClusterSize));
}
}

Expand Down Expand Up @@ -275,8 +278,8 @@ void llvm::SplitModule(
ValueToValueMapTy VMap;
std::unique_ptr<Module> MPart(
CloneModule(M, VMap, [&](const GlobalValue *GV) {
if (ClusterIDMap.count(GV))
return (ClusterIDMap[GV] == I);
if (auto It = ClusterIDMap.find(GV); It != ClusterIDMap.end())
Copy link
Contributor

Choose a reason for hiding this comment

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

For readability it would probably be better to move out the assignment of It.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it? It's a common pattern that reduces the scope of 'It':

llvm-project/clang/lib/AST/Interp/Program.cpp

Line 117 in 9d45077

if (auto It = GlobalIndices.find(P); It != GlobalIndices.end()) {
,

llvm-project/llvm/lib/IR/DebugLoc.cpp

Line 79 in 9d45077

if (auto It = Cache.find(Loc); It != Cache.end()) {
,

llvm-project/clang/lib/Analysis/FlowSensitive/Formula.cpp

Line 60 in 9d45077

if (auto It = Names->find(getAtom()); It != Names->end()) {
etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also use this pattern a lot and I think it's more readable than 2 separate line

Copy link
Contributor

Choose a reason for hiding this comment

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

Right I guess sits down to personal preference.

return It->second == I;
else
return isInPartition(GV, I, N);
}));
Expand Down
Loading