[NFC][LLVM] Namespace cleanup in ScalarEvolution

[jurahul]

No description provided.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Rahul Joshi (jurahul)

Changes

---

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

2 Files Affected:

• (modified) llvm/lib/Analysis/ScalarEvolution.cpp (+11-2)
• (modified) llvm/lib/Analysis/ScalarEvolutionDivision.cpp (-4)

diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index a31f17b1936d6..63f75ef8adbb6 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -13990,7 +13990,15 @@ static void PrintLoopInfo(raw_ostream &OS, ScalarEvolution *SE,
 }
 
 namespace llvm {
-raw_ostream &operator<<(raw_ostream &OS, ScalarEvolution::LoopDisposition LD) {
+// Note: these overloaded operators need to be in the llvm namespace for them
+// to be resolved correctly. If we put them outside the llvm namespace, the
+//
+// OS << ": " << SE.getLoopDisposition(SV, InnerL);
+//
+// code below "breaks" and start printing raw enum values as opposed to the
+// string values.
+static raw_ostream &operator<<(raw_ostream &OS,
+                               ScalarEvolution::LoopDisposition LD) {
   switch (LD) {
   case ScalarEvolution::LoopVariant:
     OS << "Variant";
@@ -14005,7 +14013,8 @@ raw_ostream &operator<<(raw_ostream &OS, ScalarEvolution::LoopDisposition LD) {
   return OS;
 }
 
-raw_ostream &operator<<(raw_ostream &OS, ScalarEvolution::BlockDisposition BD) {
+static raw_ostream &operator<<(raw_ostream &OS,
+                               llvm::ScalarEvolution::BlockDisposition BD) {
   switch (BD) {
   case ScalarEvolution::DoesNotDominateBlock:
     OS << "DoesNotDominate";
diff --git a/llvm/lib/Analysis/ScalarEvolutionDivision.cpp b/llvm/lib/Analysis/ScalarEvolutionDivision.cpp
index bce41f9f5329e..4e422539ff9f6 100644
--- a/llvm/lib/Analysis/ScalarEvolutionDivision.cpp
+++ b/llvm/lib/Analysis/ScalarEvolutionDivision.cpp
@@ -29,8 +29,6 @@ class Type;
 
 using namespace llvm;
 
-namespace {
-
 static inline int sizeOfSCEV(const SCEV *S) {
   struct FindSCEVSize {
     int Size = 0;
@@ -52,8 +50,6 @@ static inline int sizeOfSCEV(const SCEV *S) {
   return F.Size;
 }
 
-} // namespace
-
 // Computes the Quotient and Remainder of the division of Numerator by
 // Denominator.
 void SCEVDivision::divide(ScalarEvolution &SE, const SCEV *Numerator,

[jurahul]

ping @kazutakahirata (I know nikic is OOO)

[kazutakahirata]

The ScalarEvolutionDivision.cpp bits look good. I'm not sure about the ScalarEvolution.cpp bits. I generally like clean code, but if we have to add a comment to accomplish the cleanup task, we might be increasing cognitive load. The reader would have to understand the code itself and why the code is written in a particular manner.

[jurahul]

The added comment in ScalarEvolution.cpp is actually explaining the existing code (why are the << operators required to be llvm namespace, to make ADL work correctly to print the enum values in string form). The static then is just making sure these don't escape outside the file. When I initially just make them static (without llvm namespace) code broken in non-intuitive ways and I had to hunt around for why, so thought a comment here would make sense. I can add this comment as a separate PR as well since its unrelated to these being made static.

We can also wait for @nikic to weigh in as there is no rush to get this in.