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

[mingmingl-llvm]

The section prefix validation was initially added when global variable's section prefix is only added in the codegen pass.

Now the optimizer pass MemProfUse can annotate data section prefix by making use of data access profiles, so we can remove the validation. Also calls setSectionPrefix which returns whether section prefix is updated.

This is the 3rd patch as a split of #155337

[teresajohnson]

The change looks fine but should there be a test that uses the DAP profiles and triggers this?

[mingmingl-llvm]

The change looks fine but should there be a test that uses the DAP profiles and triggers this?

Yeah removing validation without test coverage is generally a 'meh' PR indeed :(

This piece of code doesn't have test coverage in the trunk-of-tree. 95292a6 adds a RUN line (not llc). I'll rebase this PR on top of that one to show the updated test.

[mingmingl-llvm]

The change looks fine but should there be a test that uses the DAP profiles and triggers this?

Yeah removing validation without test coverage is generally a 'meh' PR indeed :(

This piece of code doesn't have test coverage in the trunk-of-tree. 95292a6 adds a RUN line (not llc). I'll rebase this PR on top of that one to show the updated test.

Sorry for being back and forth. I realized I need a new llc test which process IR with section_prefix eventually.

Let me add that as a new test (under CodeGen) in the prior PR #162348 which tests not llc. After that

• This PR can flip the not --> llc should pass
• The subsequent PRs of this one can extend on the new test.

I'll ping on #162348 and this one when they are ready for review.

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: Mingming Liu (mingmingl-llvm)

Changes

The section prefix validation was initially added when global variable's section prefix is only added in the codegen pass.

Now the optimizer pass MemProfUse can annotate data section prefix by making use of data access profiles, so we can remove the validation. Also calls setSectionPrefix which returns whether section prefix is updated.

This is the 3rd patch as a split of #155337

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/StaticDataAnnotator.cpp (+1-12)
• (modified) llvm/test/CodeGen/X86/global-variable-partition-with-dap.ll (+5-5)

diff --git a/llvm/lib/CodeGen/StaticDataAnnotator.cpp b/llvm/lib/CodeGen/StaticDataAnnotator.cpp
index 9b737751c4a98..eac2012017087 100644
--- a/llvm/lib/CodeGen/StaticDataAnnotator.cpp
+++ b/llvm/lib/CodeGen/StaticDataAnnotator.cpp
@@ -78,19 +78,8 @@ bool StaticDataAnnotator::runOnModule(Module &M) {
     if (!llvm::memprof::IsAnnotationOK(GV))
       continue;
 
-    // The implementation below assumes prior passes don't set section prefixes,
-    // and specifically do 'assign' rather than 'update'. So report error if a
-    // section prefix is already set.
-    if (auto maybeSectionPrefix = GV.getSectionPrefix();
-        maybeSectionPrefix && !maybeSectionPrefix->empty())
-      llvm::report_fatal_error("Global variable " + GV.getName() +
-                               " already has a section prefix " +
-                               *maybeSectionPrefix);
-
     StringRef SectionPrefix = SDPI->getConstantSectionPrefix(&GV, PSI);
-    if (SectionPrefix.empty())
-      continue;
-
+    // setSectionPrefix returns true if the section prefix is updated.
     Changed |= GV.setSectionPrefix(SectionPrefix);
   }
 
diff --git a/llvm/test/CodeGen/X86/global-variable-partition-with-dap.ll b/llvm/test/CodeGen/X86/global-variable-partition-with-dap.ll
index 7214273ff6d9c..f3950b75a969f 100644
--- a/llvm/test/CodeGen/X86/global-variable-partition-with-dap.ll
+++ b/llvm/test/CodeGen/X86/global-variable-partition-with-dap.ll
@@ -1,15 +1,15 @@
 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
-;; prefix. Subsequent PRs will expand on this test case to test the hotness
+;; A minimal test case. Subsequent PRs will expand on this test case
+;; (e.g., with more functions, variables and profiles) and test the hotness
 ;; reconcillation implementation.
-; RUN: not llc -mtriple=x86_64-unknown-linux-gnu -relocation-model=pic \
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -relocation-model=pic \
 ; RUN:     -partition-static-data-sections=true \
 ; RUN:     -data-sections=true  -unique-section-names=false \
-; RUN:     %s -o - 2>&1 | FileCheck %s --check-prefix=ERR
+; RUN:     %s -o - 2>&1 | FileCheck %s --check-prefix=IR
 
-; ERR: Global variable hot_bss already has a section prefix hot
+; IR: .section .bss.hot.,"aw"
 
 @hot_bss = internal global i32 0, !section_prefix !17
 

[mingmingl-llvm]

I'll ping on #162348 and this one when they are ready for review.

This one is ready for review. PTAL, thanks!

[teresajohnson]

lgtm