[StaticDataLayout] Factor out a helper function for section prefix eligibility and use it in both optimizer and codegen #162348

mingmingl-llvm · 2025-10-07T19:19:19Z

This change introduces new helper functions to check if a global variable is eligible for section prefix annotation.

This shared logic is used by both MemProfUse and StaticDataSplitter to avoid annotating ineligible variables.

This is the 2nd patch as a split of #155337

…tatus of a global variable for section prefix annotation

llvmbot · 2025-10-07T19:22:09Z

@llvm/pr-subscribers-pgo
@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Mingming Liu (mingmingl-llvm)

Changes

This change introduces new helper functions to check if a global variable is eligible for section prefix annotation.

This shared logic will be used by both MemProfUse and StaticDataSplitter to avoid annotating ineligible variables.

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

4 Files Affected:

(modified) llvm/include/llvm/Analysis/StaticDataProfileInfo.h (+18)
(modified) llvm/lib/Analysis/StaticDataProfileInfo.cpp (+37)
(modified) llvm/lib/CodeGen/StaticDataAnnotator.cpp (+1-1)
(modified) llvm/lib/Transforms/Instrumentation/MemProfUse.cpp (+8-22)

diff --git a/llvm/include/llvm/Analysis/StaticDataProfileInfo.h b/llvm/include/llvm/Analysis/StaticDataProfileInfo.h
index fa21eba1377df..f06e7ceaa74ce 100644
--- a/llvm/include/llvm/Analysis/StaticDataProfileInfo.h
+++ b/llvm/include/llvm/Analysis/StaticDataProfileInfo.h
@@ -10,6 +10,24 @@
 
 namespace llvm {
 
+namespace memprof {
+// Represents the eligibility status of a global variable for section prefix
+// annotation. Other than AnnotationOk, each enum value indicates a specific
+// reason for ineligibility.
+enum class AnnotationKind : uint8_t {
+  AnnotationOK,
+  DeclForLinker,
+  ExplicitSection,
+  ReservedName,
+};
+/// Returns the annotation kind of the global variable \p GV.
+AnnotationKind getAnnotationKind(const GlobalVariable &GV);
+
+/// Returns true if the annotation kind of the global variable \p GV is
+/// AnnotationOK.
+bool IsAnnotationOK(const GlobalVariable &GV);
+} // namespace memprof
+
 /// A class that holds the constants that represent static data and their
 /// profile information and provides methods to operate on them.
 class StaticDataProfileInfo {
diff --git a/llvm/lib/Analysis/StaticDataProfileInfo.cpp b/llvm/lib/Analysis/StaticDataProfileInfo.cpp
index b036b2dde770e..ff4582ca7eeb1 100644
--- a/llvm/lib/Analysis/StaticDataProfileInfo.cpp
+++ b/llvm/lib/Analysis/StaticDataProfileInfo.cpp
@@ -6,6 +6,43 @@
 #include "llvm/ProfileData/InstrProf.h"
 
 using namespace llvm;
+
+namespace llvm {
+namespace memprof {
+// Returns true iff the global variable has custom section either by
+// __attribute__((section("name")))
+// (https://clang.llvm.org/docs/AttributeReference.html#section-declspec-allocate)
+// or #pragma clang section directives
+// (https://clang.llvm.org/docs/LanguageExtensions.html#specifying-section-names-for-global-objects-pragma-clang-section).
+static bool hasExplicitSectionName(const GlobalVariable &GVar) {
+  if (GVar.hasSection())
+    return true;
+
+  auto Attrs = GVar.getAttributes();
+  if (Attrs.hasAttribute("bss-section") || Attrs.hasAttribute("data-section") ||
+      Attrs.hasAttribute("relro-section") ||
+      Attrs.hasAttribute("rodata-section"))
+    return true;
+  return false;
+}
+
+AnnotationKind getAnnotationKind(const GlobalVariable &GV) {
+  if (GV.isDeclarationForLinker())
+    return AnnotationKind::DeclForLinker;
+  StringRef Name = GV.getName();
+  if (Name.starts_with("llvm."))
+    return AnnotationKind::ReservedName;
+  if (hasExplicitSectionName(GV))
+    return AnnotationKind::ExplicitSection;
+  return AnnotationKind::AnnotationOK;
+}
+
+bool IsAnnotationOK(const GlobalVariable &GV) {
+  return getAnnotationKind(GV) == AnnotationKind::AnnotationOK;
+}
+} // namespace memprof
+} // namespace llvm
+
 void StaticDataProfileInfo::addConstantProfileCount(
     const Constant *C, std::optional<uint64_t> Count) {
   if (!Count) {
diff --git a/llvm/lib/CodeGen/StaticDataAnnotator.cpp b/llvm/lib/CodeGen/StaticDataAnnotator.cpp
index 53a9ab4dbda02..9b737751c4a98 100644
--- a/llvm/lib/CodeGen/StaticDataAnnotator.cpp
+++ b/llvm/lib/CodeGen/StaticDataAnnotator.cpp
@@ -75,7 +75,7 @@ bool StaticDataAnnotator::runOnModule(Module &M) {
 
   bool Changed = false;
   for (auto &GV : M.globals()) {
-    if (GV.isDeclarationForLinker())
+    if (!llvm::memprof::IsAnnotationOK(GV))
       continue;
 
     // The implementation below assumes prior passes don't set section prefixes,
diff --git a/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp b/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
index d86fcf268ce4f..ca2af1a9534d3 100644
--- a/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
@@ -17,6 +17,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Analysis/MemoryProfileInfo.h"
 #include "llvm/Analysis/OptimizationRemarkEmitter.h"
+#include "llvm/Analysis/StaticDataProfileInfo.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
@@ -775,23 +776,6 @@ PreservedAnalyses MemProfUsePass::run(Module &M, ModuleAnalysisManager &AM) {
   return PreservedAnalyses::none();
 }
 
-// Returns true iff the global variable has custom section either by
-// __attribute__((section("name")))
-// (https://clang.llvm.org/docs/AttributeReference.html#section-declspec-allocate)
-// or #pragma clang section directives
-// (https://clang.llvm.org/docs/LanguageExtensions.html#specifying-section-names-for-global-objects-pragma-clang-section).
-static bool hasExplicitSectionName(const GlobalVariable &GVar) {
-  if (GVar.hasSection())
-    return true;
-
-  auto Attrs = GVar.getAttributes();
-  if (Attrs.hasAttribute("bss-section") || Attrs.hasAttribute("data-section") ||
-      Attrs.hasAttribute("relro-section") ||
-      Attrs.hasAttribute("rodata-section"))
-    return true;
-  return false;
-}
-
 bool MemProfUsePass::annotateGlobalVariables(
     Module &M, const memprof::DataAccessProfData *DataAccessProf) {
   if (!AnnotateStaticDataSectionPrefix || M.globals().empty())
@@ -817,13 +801,16 @@ bool MemProfUsePass::annotateGlobalVariables(
   for (GlobalVariable &GVar : M.globals()) {
     assert(!GVar.getSectionPrefix().has_value() &&
            "GVar shouldn't have section prefix yet");
-    if (GVar.isDeclarationForLinker())
-      continue;
-
-    if (hasExplicitSectionName(GVar)) {
+    auto Kind = llvm::memprof::getAnnotationKind(GVar);
+    switch (Kind) {
+    case llvm::memprof::AnnotationKind::AnnotationOK:
+      break;
+    case llvm::memprof::AnnotationKind::ExplicitSection:
       ++NumOfMemProfExplicitSectionGlobalVars;
       LLVM_DEBUG(dbgs() << "Global variable " << GVar.getName()
                         << " has explicit section name. Skip annotating.\n");
+      [[fallthrough]];
+    default:
       continue;
     }
 
@@ -833,7 +820,6 @@ bool MemProfUsePass::annotateGlobalVariables(
     // TODO: Track string content hash in the profiles and compute it inside the
     // compiler to categeorize the hotness string literals.
     if (Name.starts_with(".str")) {
-
       LLVM_DEBUG(dbgs() << "Skip annotating string literal " << Name << "\n");
       continue;
     }

teresajohnson · 2025-10-07T21:21:02Z

llvm/lib/CodeGen/StaticDataAnnotator.cpp

  bool Changed = false;
  for (auto &GV : M.globals()) {
-    if (GV.isDeclarationForLinker())
+    if (!llvm::memprof::IsAnnotationOK(GV))


This helper function includes more checks than just isDeclarationForLinker - so is this change really NFC?

yeah I agree.

I think this PR is more of 'NFCI' (non functional change intended) or no user-visible change intended. I removed the 'NFC' tag so (unfamiliar) readers won't get (unintentionally) fooled..

But are the effects of this change and the other one I mentioned earlier not observable by tests?

But are the effects of this change and the other one I mentioned earlier not observable by tests?

Yeah, the test coverage is limited to [1] before. Improved the test coverage now, PTAL, thanks!

[1]

llvm-project/llvm/test/Transforms/PGOProfile/data-access-profile.ll

Lines 28 to 29 in 139a6bf

; LOG: Global variable var3 has explicit section name. Skip annotating.

; LOG: Global variable var4 has explicit section name. Skip annotating.

teresajohnson · 2025-10-07T21:26:10Z

llvm/lib/Analysis/StaticDataProfileInfo.cpp

+  if (GV.isDeclarationForLinker())
+    return AnnotationKind::DeclForLinker;
+  StringRef Name = GV.getName();
+  if (Name.starts_with("llvm."))


This check is new afaict - so is this NFC?

ah sorry, you are correct.

This is from another similar call site [1] and this PR means to cover that one. Updated the PR.

[1]

llvm-project/llvm/lib/CodeGen/StaticDataSplitter.cpp

Lines 133 to 137 in 30b9ef8

// Skip 'llvm.'-prefixed global variables conservatively because they are

// often handled specially, and skip those not in static data

// sections.

if (!GV || GV->getName().starts_with("llvm.") ||

!inStaticDataSection(*GV, TM))

mingmingl-llvm

thanks for reviews! PTAL.

mingmingl-llvm · 2025-10-07T21:42:16Z

llvm/lib/Analysis/StaticDataProfileInfo.cpp

+  if (GV.isDeclarationForLinker())
+    return AnnotationKind::DeclForLinker;
+  StringRef Name = GV.getName();
+  if (Name.starts_with("llvm."))


ah sorry, you are correct.

This is from another similar call site [1] and this PR means to cover that one. Updated the PR.

[1]

llvm-project/llvm/lib/CodeGen/StaticDataSplitter.cpp

Lines 133 to 137 in 30b9ef8

// Skip 'llvm.'-prefixed global variables conservatively because they are

// often handled specially, and skip those not in static data

// sections.

if (!GV || GV->getName().starts_with("llvm.") ||

!inStaticDataSection(*GV, TM))

mingmingl-llvm · 2025-10-07T21:44:53Z

llvm/lib/CodeGen/StaticDataAnnotator.cpp

  bool Changed = false;
  for (auto &GV : M.globals()) {
-    if (GV.isDeclarationForLinker())
+    if (!llvm::memprof::IsAnnotationOK(GV))


yeah I agree.

I think this PR is more of 'NFCI' (non functional change intended) or no user-visible change intended. I removed the 'NFC' tag so (unfamiliar) readers won't get (unintentionally) fooled..

This reverts commit 95292a6.

mingmingl-llvm · 2025-10-08T23:57:33Z

Hi folks, this PR is ready for review (again, after llvm/test/CodeGen/X86/global-variable-partition-with-dap.ll is added for subsequent PRs to expand on), thanks!

teresajohnson

lgtm with a few small fixes below

teresajohnson · 2025-10-09T14:52:19Z

llvm/test/CodeGen/X86/global-variable-partition-with-dap.ll

+
+;; A minimal test case. llc will crash if global variables already has a section
+;; prefix. Subsequent PRs will expand on this test case to test the hotness
+;; reconcillation implementation.


s/reconcillation/reconciliation/

fixed in #162938 and will rebase the PR that depends on the test.

teresajohnson · 2025-10-09T15:03:23Z

llvm/test/CodeGen/X86/global-variable-partition-with-dap.ll

+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+;; A minimal test case. llc will crash if global variables already has a section


Is the behavior of this test affected by this PR? If not, commit it separately as a standalone change.

done in #162938 to pre-commit the test case.

teresajohnson · 2025-10-09T15:08:29Z

llvm/lib/Transforms/Instrumentation/MemProfUse.cpp

-      LLVM_DEBUG(dbgs() << "Global variable " << GVar.getName()
-                        << " has explicit section name. Skip annotating.\n");
+    auto Kind = llvm::memprof::getAnnotationKind(GVar);
+    switch (Kind) {


Since the switch now only handles 2 cases, this would be clearer written as an if condition like:

if (Kind != llvm::memprof::AnnotationKind::AnnotationOK) { HandleUnsupportedAnnotationKinds(GVar, Kind); continue; }

I'll resolve this one shortly after.

done in 34b47c9

mingmingl-llvm

thanks for reviews!

mingmingl-llvm · 2025-10-10T22:40:11Z

llvm/test/CodeGen/X86/global-variable-partition-with-dap.ll

+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+;; A minimal test case. llc will crash if global variables already has a section


done in #162938 to pre-commit the test case.

mingmingl-llvm · 2025-10-10T22:41:18Z

llvm/test/CodeGen/X86/global-variable-partition-with-dap.ll

+
+;; A minimal test case. llc will crash if global variables already has a section
+;; prefix. Subsequent PRs will expand on this test case to test the hotness
+;; reconcillation implementation.


fixed in #162938 and will rebase the PR that depends on the test.

…162938)

mingmingl-llvm added 2 commits October 7, 2025 11:59

[nfc][StaticDataLayout]Add helper functions to tell the eligibility s…

f68b06f

…tatus of a global variable for section prefix annotation

save irrelevant change for the next PR to make this one focused

331888b

mingmingl-llvm requested review from snehasish and teresajohnson and removed request for snehasish October 7, 2025 19:21

mingmingl-llvm marked this pull request as ready for review October 7, 2025 19:21

llvmbot added llvm:codegen llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Oct 7, 2025

teresajohnson reviewed Oct 7, 2025

View reviewed changes

mingmingl-llvm changed the title ~~[nfc][StaticDataLayout] Factor out helper functions for section prefix eligibility~~ [StaticDataLayout] Factor out a helper function for section prefix eligibility and use it in both optimizer and codegen Oct 7, 2025

resolve comments

dcb9f00

mingmingl-llvm commented Oct 7, 2025

View reviewed changes

mingmingl-llvm requested a review from teresajohnson October 7, 2025 21:45

add test coverage

3565bc0

llvmbot added backend:X86 PGO Profile Guided Optimizations labels Oct 8, 2025

mingmingl-llvm added 2 commits October 8, 2025 15:48

simplify test

905b80f

test coverage for the subsequent patch #162349

95292a6

mingmingl-llvm mentioned this pull request Oct 8, 2025

[NFCI][StaticDataLayout] Get rid of section prefix validation in codegen #162349

Open

mingmingl-llvm added 2 commits October 8, 2025 16:46

Revert "test coverage for the subsequent patch #162349"

64be09f

This reverts commit 95292a6.

add a minial test case for subsequent PRs to expand on

4a90c77

one liner fix in test

f9c6266

teresajohnson approved these changes Oct 9, 2025

View reviewed changes

mingmingl-llvm added a commit that referenced this pull request Oct 10, 2025

Pre-commit test case for #162348

98ba624

mingmingl-llvm commented Oct 10, 2025

View reviewed changes

simplify switch-break to if-continue

34b47c9

mingmingl-llvm added a commit that referenced this pull request Oct 10, 2025

[NFC] Pre-commit test case for #162348 (#162938)

0aef9eb

rebase on top of pre-commit test in #162348

b880ee3

mingmingl-llvm added a commit that referenced this pull request Oct 10, 2025

rebase for pre-commit test case in #162348

41f3d6a

llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Oct 10, 2025

Automerge: [NFC] Pre-commit test case for llvm/llvm-project#162348 (#…

1ff88a4

…162938)

	; LOG: Global variable var3 has explicit section name. Skip annotating.
	; LOG: Global variable var4 has explicit section name. Skip annotating.

	// Skip 'llvm.'-prefixed global variables conservatively because they are
	// often handled specially, and skip those not in static data
	// sections.
	if (!GV \|\| GV->getName().starts_with("llvm.") \|\|
	!inStaticDataSection(*GV, TM))

[StaticDataLayout] Factor out a helper function for section prefix eligibility and use it in both optimizer and codegen #162348

Are you sure you want to change the base?

[StaticDataLayout] Factor out a helper function for section prefix eligibility and use it in both optimizer and codegen #162348

Conversation

mingmingl-llvm commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm commented Oct 8, 2025

Uh oh!

teresajohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mingmingl-llvm commented Oct 7, 2025 •

edited

Loading

llvmbot commented Oct 7, 2025 •

edited

Loading