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

sergachev · 2024-06-18T09:49:12Z

Fix repeated map lookup, variable name, formatting and a missing space.

github-actions · 2024-06-18T09:49:35Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-06-18T09:50:06Z

@llvm/pr-subscribers-llvm-transforms

Author: Ilia Sergachev (sergachev)

Changes

Fix repeated map lookup, variable name, formatting and a missing space.

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

1 Files Affected:

(modified) llvm/lib/Transforms/Utils/SplitModule.cpp (+14-11)

diff --git a/llvm/lib/Transforms/Utils/SplitModule.cpp b/llvm/lib/Transforms/Utils/SplitModule.cpp
index 9c39c26d8b7af..6fa0d69c29bd6 100644
--- a/llvm/lib/Transforms/Utils/SplitModule.cpp
+++ b/llvm/lib/Transforms/Utils/SplitModule.cpp
@@ -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;
 
@@ -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>;
 
@@ -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)
@@ -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()
@@ -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));
   }
 }
 
@@ -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())
+            return (It->second == I);
           else
             return isInPartition(GV, I, N);
         }));

llvm/lib/Transforms/Utils/SplitModule.cpp

fhahn · 2024-06-25T09:52:46Z

llvm/lib/Transforms/Utils/SplitModule.cpp

@@ -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())


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

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.

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

Right I guess sits down to personal preference.

sergachev · 2024-07-02T08:41:16Z

ping

Pierre-vh · 2024-07-02T09:30:36Z

llvm/lib/Transforms/Utils/SplitModule.cpp

@@ -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())


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

fhahn

LGTM, thanks!

fhahn · 2024-07-02T09:36:22Z

llvm/lib/Transforms/Utils/SplitModule.cpp

@@ -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())


Right I guess sits down to personal preference.

github-actions · 2024-07-02T10:30:43Z

@sergachev Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvm-ci · 2024-07-02T11:06:56Z

LLVM Buildbot has detected a new failure on builder sanitizer-windows running on sanitizer-windows while building llvm at step 4 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/107/builds/587

Here is the relevant piece of the build log for the reference:

Step 4 (annotate) failure: 'python ../llvm-zorg/zorg/buildbot/builders/annotated/sanitizer-windows.py ...' (failure)
...
[18/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\InterpBuiltin.cpp.obj
[19/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\Context.cpp.obj
[20/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\EvalEmitter.cpp.obj
[21/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\Compiler.cpp.obj
[22/30] Building CXX object lib\CodeGen\AsmPrinter\CMakeFiles\LLVMAsmPrinter.dir\AsmPrinter.cpp.obj
[23/30] Linking CXX static library lib\LLVMAsmPrinter.lib
[24/30] Building CXX object lib\LTO\CMakeFiles\LLVMLTO.dir\LTO.cpp.obj
[25/30] Linking CXX static library lib\LLVMLTO.lib
[26/30] Linking CXX executable bin\lld.exe
[27/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\Disasm.cpp.obj
command timed out: 1200 seconds without output running ['python', '../llvm-zorg/zorg/buildbot/builders/annotated/sanitizer-windows.py', '--jobs=16'], attempting to kill
program finished with exit code 1
elapsedTime=1273.595000
Step 7 (stage 1 build) failure: stage 1 build (failure)
@@@BUILD_STEP stage 1 build@@@
Running: ninja -j 16 compiler-rt
[1/2] Building CXX object projects\compiler-rt\lib\asan\CMakeFiles\RTAsan_dynamic_version_script_dummy.x86_64.dir\dummy.cpp.obj
[2/2] Linking CXX shared library lib\clang\19\lib\windows\clang_rt.asan_dynamic-x86_64.dll
Running: ninja -j 16 clang lld
[1/30] Building Opcodes.inc...
[2/30] Generating VCSRevision.h
[3/30] Generating VCSVersion.inc
[4/30] Building CXX object tools\clang\lib\Basic\CMakeFiles\obj.clangBasic.dir\Version.cpp.obj
[5/30] Building CXX object lib\Transforms\Utils\CMakeFiles\LLVMTransformUtils.dir\SplitModule.cpp.obj
[6/30] Building CXX object lib\Object\CMakeFiles\LLVMObject.dir\IRSymtab.cpp.obj
[7/30] Linking CXX static library lib\LLVMObject.lib
[8/30] Linking CXX static library lib\LLVMTransformUtils.lib
[9/30] Building CXX object lib\Transforms\Scalar\CMakeFiles\LLVMScalarOpts.dir\CorrelatedValuePropagation.cpp.obj
[10/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\Function.cpp.obj
[11/30] Linking CXX static library lib\LLVMScalarOpts.lib
[12/30] Generating VCSVersion.inc
[13/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\Program.cpp.obj
[14/30] Linking CXX static library lib\clangBasic.lib
[15/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\ByteCodeEmitter.cpp.obj
[16/30] Building CXX object tools\lld\Common\CMakeFiles\lldCommon.dir\Version.cpp.obj
[17/30] Linking CXX static library lib\lldCommon.lib
[18/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\InterpBuiltin.cpp.obj
[19/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\Context.cpp.obj
[20/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\EvalEmitter.cpp.obj
[21/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\Compiler.cpp.obj
[22/30] Building CXX object lib\CodeGen\AsmPrinter\CMakeFiles\LLVMAsmPrinter.dir\AsmPrinter.cpp.obj
[23/30] Linking CXX static library lib\LLVMAsmPrinter.lib
[24/30] Building CXX object lib\LTO\CMakeFiles\LLVMLTO.dir\LTO.cpp.obj
[25/30] Linking CXX static library lib\LLVMLTO.lib
[26/30] Linking CXX executable bin\lld.exe
[27/30] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\Interp\Disasm.cpp.obj

command timed out: 1200 seconds without output running ['python', '../llvm-zorg/zorg/buildbot/builders/annotated/sanitizer-windows.py', '--jobs=16'], attempting to kill
program finished with exit code 1
elapsedTime=1273.595000

Fix repeated map lookup, variable name, formatting and a missing space.

[Transforms][NFC] Tiny fixes in SplitModule

8456f2f

llvmbot added the llvm:transforms label Jun 18, 2024

dtcxzyw requested review from fhahn and Pierre-vh June 19, 2024 02:50

fhahn reviewed Jun 25, 2024

View reviewed changes

Remove unnecessary parentheses

8ef43a8

Pierre-vh approved these changes Jul 2, 2024

View reviewed changes

fhahn approved these changes Jul 2, 2024

View reviewed changes

grypp merged commit b7e157c into llvm:main Jul 2, 2024
7 checks passed

sergachev deleted the users/sergachev/cleanup_splitmodule branch July 2, 2024 10:44

lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024

[Transforms][NFC] Tiny fixes in SplitModule (llvm#95903)

fe0b28c

Fix repeated map lookup, variable name, formatting and a missing space.

kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024

[Transforms][NFC] Tiny fixes in SplitModule (llvm#95903)

b04af7c

Fix repeated map lookup, variable name, formatting and a missing space.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

sergachev commented Jun 18, 2024

Uh oh!

github-actions bot commented Jun 18, 2024

Uh oh!

llvmbot commented Jun 18, 2024

Uh oh!

Uh oh!

fhahn Jun 25, 2024

Uh oh!

sergachev Jun 25, 2024

Uh oh!

Pierre-vh Jul 2, 2024

Uh oh!

fhahn Jul 2, 2024

Uh oh!

sergachev commented Jul 2, 2024

Uh oh!

Pierre-vh Jul 2, 2024

Uh oh!

fhahn left a comment

Uh oh!

fhahn Jul 2, 2024

Uh oh!

Uh oh!

github-actions bot commented Jul 2, 2024

Uh oh!

llvm-ci commented Jul 2, 2024

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

sergachev commented Jun 18, 2024

Uh oh!

github-actions bot commented Jun 18, 2024

Uh oh!

llvmbot commented Jun 18, 2024

Uh oh!

Uh oh!

fhahn Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

sergachev Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

Pierre-vh Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

fhahn Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

sergachev commented Jul 2, 2024

Uh oh!

Pierre-vh Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

fhahn Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jul 2, 2024

Uh oh!

llvm-ci commented Jul 2, 2024

Uh oh!

Uh oh!