[NFC][LLVM] Fix some llvm namespace usage in Bitcode writer

[jurahul]

• Add declaration of ForceSummaryEdgesCold to ModuleSummaryAnalysis.h and do not use namespace scope for its definition in ModuleSummaryAnalysis.cpp
• No need of forward declaration of Argument.
• include ModuleSummaryAnalysis.h in BitcodeWriter.cpp to get declaration of ForceSummaryEdgesCold

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Rahul Joshi (jurahul)

Changes

• Add declaration of ForceSummaryEdgesCold to ModuleSummaryAnalysis.h and do not use namespace scope for its definition in ModuleSummaryAnalysis.cpp
• No need of forward declaration of Argument.
• include ModuleSummaryAnalysis.h in BitcodeWriter.cpp to get declaration of ForceSummaryEdgesCold

---

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

4 Files Affected:

• (modified) llvm/include/llvm/Analysis/ModuleSummaryAnalysis.h (+2)
• (modified) llvm/lib/Analysis/ModuleSummaryAnalysis.cpp (+1-3)
• (modified) llvm/lib/Bitcode/Reader/MetadataLoader.cpp (-3)
• (modified) llvm/lib/Bitcode/Writer/BitcodeWriter.cpp (+1-4)

diff --git a/llvm/include/llvm/Analysis/ModuleSummaryAnalysis.h b/llvm/include/llvm/Analysis/ModuleSummaryAnalysis.h
index e36dea58cec45..44b5b7ebea935 100644
--- a/llvm/include/llvm/Analysis/ModuleSummaryAnalysis.h
+++ b/llvm/include/llvm/Analysis/ModuleSummaryAnalysis.h
@@ -103,6 +103,8 @@ createImmutableModuleSummaryIndexWrapperPass(const ModuleSummaryIndex *Index);
 /// consistency between summary analysis and the ThinLTO backend processing.
 bool mayHaveMemprofSummary(const CallBase *CB);
 
+extern FunctionSummary::ForceSummaryHotnessType ForceSummaryEdgesCold;
+
 } // end namespace llvm
 
 #endif // LLVM_ANALYSIS_MODULESUMMARYANALYSIS_H
diff --git a/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp b/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
index d7e12dc80d7c2..571212c442689 100644
--- a/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
+++ b/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
@@ -63,10 +63,8 @@ using namespace llvm::memprof;
 
 // Option to force edges cold which will block importing when the
 // -import-cold-multiplier is set to 0. Useful for debugging.
-namespace llvm {
-FunctionSummary::ForceSummaryHotnessType ForceSummaryEdgesCold =
+FunctionSummary::ForceSummaryHotnessType llvm::ForceSummaryEdgesCold =
     FunctionSummary::FSHT_None;
-} // namespace llvm
 
 static cl::opt<FunctionSummary::ForceSummaryHotnessType, true> FSEC(
     "force-summary-edges-cold", cl::Hidden, cl::location(ForceSummaryEdgesCold),
diff --git a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
index c724488b45d48..8f67f00232fcd 100644
--- a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
+++ b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
@@ -60,9 +60,6 @@
 #include <type_traits>
 #include <utility>
 #include <vector>
-namespace llvm {
-class Argument;
-}
 
 using namespace llvm;
 
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index f8d80c1fc7798..5b3cd59f40821 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -23,6 +23,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Analysis/ModuleSummaryAnalysis.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/Bitcode/BitcodeCommon.h"
 #include "llvm/Bitcode/BitcodeReader.h"
@@ -117,10 +118,6 @@ static cl::opt<bool>
 #endif
                                 cl::desc(""));
 
-namespace llvm {
-extern FunctionSummary::ForceSummaryHotnessType ForceSummaryEdgesCold;
-}
-
 extern llvm::cl::opt<bool> UseNewDbgInfoFormat;
 
 namespace {

[teresajohnson]

I'm not sure including an additional header is superior to just including the extern here below - why make this change?

[jurahul]

This is an attempt to apply https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions to variables as well. I have another PR for the coding standard change (not yet approved).

Also, the idea that there is just one declaration in a header shared by all who access that variables.

[teresajohnson]

Thanks for the background. I'm not sure how I feel about going through and changing all of these as I think it will cause a lot of churn and add compile time. Let's wait to see if the coding standard change is accepted.

[teresajohnson]

I suppose this could be simplified to "extern llvm::FunctionSummary::..." like the extern llvm::cl::opt below, if we decide to keep it.

[jurahul]

Unfortunately, that does not work. Note in the case below, llvm:: is for the type. Here we want to declare an extern variable in the llvm namespace, and extern type llvm::VarName does not work (I did try that first).

[teresajohnson]

Thanks for the background. I'm not sure how I feel about going through and changing all of these as I think it will cause a lot of churn and add compile time. Let's wait to see if the coding standard change is accepted.

[teresajohnson]

This one is unrelated to the other changes and can be split into a separate PR and committed. There is a use of Argument in this file, but presumably it is being pulled in via other headers, so no need for this forward def.