[NFCI][Globals] In GlobalObjects::setSectionPrefix, do conditional update if existing prefix is not equivalent to the new one. Returns whether prefix changed. #158460

mingmingl-llvm · 2025-09-14T06:17:56Z

Before this change, setSectionPrefix overwrites existing section prefix with new one unconditionally.

After this change, setSectionPrefix checks for equivalences, updates conditionally and returns whether an update happens.

Update the existing callers to make use of the return value. PR 155337 is a motivating use case whether the 'update' semantic is needed.

…ectionPrefix to handle empty strings

llvm/include/llvm/IR/GlobalObject.h

snehasish · 2025-09-14T06:51:10Z

llvm/unittests/IR/GlobalObjectTest.cpp

+  Foo->setSectionPrefix("unlikely");
+  EXPECT_THAT(Foo->getSectionPrefix(), Optional(StrEq("unlikely")));
+
+  // Update prefix to empty is the same as clear.


s/Update/Set since we have a method called Update too?

llvm/unittests/IR/GlobalObjectTest.cpp

snehasish · 2025-09-14T06:53:05Z

llvm/unittests/IR/GlobalObjectTest.cpp

+//===----------------------------------------------------------------------===//
+
+#include "llvm/IR/GlobalObject.h"
+#include "llvm-c/Core.h"


I wonder what this header is used for?

The headers are copied from FunctionTest.cpp. I removed the unused ones now.

teresajohnson

The title and summary are both mechanical. Maybe add a 1 sentence description of why this change is needed?

teresajohnson · 2025-09-15T14:37:34Z

llvm/lib/IR/Globals.cpp

 }

+bool GlobalObject::updateSectionPrefix(StringRef Prefix) {
+  auto MD = getMetadata(LLVMContext::MD_section_prefix);


Use getSectionPrefix instead? It does some assertion checking that could be useful.

Also, does it make sense to just make this the default behavior of setSectionPrefix?

Use getSectionPrefix instead? It does some assertion checking that could be useful.

done.

does it make sense to just make this the default behavior of setSectionPrefix?

Implementation wise, the update method currently calls the set method (the other way around). Technically, no existing caller of 'set' depends on the behavior that 'set' method will overwrite, so either way (with a brief comment on the function decl in header) is fine imo. Doing it the current way will avoid affecting (fwiw) existing user in codegen prepare to set function section prefix though.

does it make sense to just make this the default behavior of setSectionPrefix?

Implementation wise, the update method currently calls the set method (the other way around). Technically, no existing caller of 'set' depends on the behavior that 'set' method will overwrite, so either way (with a brief comment on the function decl in header) is fine imo. Doing it the current way will avoid affecting (fwiw) existing user in codegen prepare to set function section prefix though.

How would existing code be affected if setSectionPrefix was changed to do the update? What is the current behavior if setSectionPrefix is used in a context where a different section prefix already exists?

How would existing code be affected if setSectionPrefix was changed to do the update?

The existing code calls 'set' once for function (callsite) or global variable (callsite). If setSectionPrefix is changed to call update, the outcome is the same, except that they get additional comparisons (and makes a difference to compile time in cases where additional comparisons are not needed, like for functions).

What is the current behavior if setSectionPrefix is used in a context where a different section prefix already exists?

It'll overwrite the existing section prefix per implementation, although existing callers calls this function once, so either an update or not works (in terms of the outcome).

Per offline discussion, the question is more about why not changing setSectionPrefix in place rather than calling update inside set.

Technically changing setSectionPrefix is definitely an option, but I previously chose to add update to not affect existing callers.

That said, compile-time changes are trivial enough, and all existing use cases also tracks whether a change happens on the IR/MIR, so I just update the PR to change setSectionPrefix in place for simplicity. One other notable change is that codegenprepare previously doesn't track function prefix change as EverMadeChange, but this patch changes it to be. PTAL, thanks!

llvmbot · 2025-09-15T22:48:49Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Mingming Liu (mingmingl-llvm)

Changes

This PR introduces two main changes to GlobalObject with unit test coverage.

Add method updateSectionPrefix method: This method allows updating the section prefix of a GlobalObject.
Changes method setSectionPrefix: when the input is an empty string, clear section_prefix metadata.

This is a split of #155337

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

4 Files Affected:

(modified) llvm/include/llvm/IR/GlobalObject.h (+6-1)
(modified) llvm/lib/IR/Globals.cpp (+17)
(modified) llvm/unittests/IR/CMakeLists.txt (+1)
(added) llvm/unittests/IR/GlobalObjectTest.cpp (+76)

diff --git a/llvm/include/llvm/IR/GlobalObject.h b/llvm/include/llvm/IR/GlobalObject.h
index 08a02b42bdc14..740d7fb9bc41f 100644
--- a/llvm/include/llvm/IR/GlobalObject.h
+++ b/llvm/include/llvm/IR/GlobalObject.h
@@ -121,9 +121,14 @@ class GlobalObject : public GlobalValue {
   /// appropriate default object file section.
   LLVM_ABI void setSection(StringRef S);
 
-  /// Set the section prefix for this global object.
+  /// Set the section prefix for this global object. If \p Prefix is empty,
+  /// the section prefix metadata will be cleared if it exists.
   LLVM_ABI void setSectionPrefix(StringRef Prefix);
 
+  /// If \p Prefix is different from existing prefix, update section prefix.
+  /// Returns true if an update happens and false otherwise.
+  LLVM_ABI bool updateSectionPrefix(StringRef Prefix);
+
   /// Get the section prefix for this global object.
   LLVM_ABI std::optional<StringRef> getSectionPrefix() const;
 
diff --git a/llvm/lib/IR/Globals.cpp b/llvm/lib/IR/Globals.cpp
index 11d33e262fecb..0731fcbde106a 100644
--- a/llvm/lib/IR/Globals.cpp
+++ b/llvm/lib/IR/Globals.cpp
@@ -289,11 +289,28 @@ void GlobalObject::setSection(StringRef S) {
 }
 
 void GlobalObject::setSectionPrefix(StringRef Prefix) {
+  if (Prefix.empty()) {
+    setMetadata(LLVMContext::MD_section_prefix, nullptr);
+    return;
+  }
   MDBuilder MDB(getContext());
   setMetadata(LLVMContext::MD_section_prefix,
               MDB.createGlobalObjectSectionPrefix(Prefix));
 }
 
+bool GlobalObject::updateSectionPrefix(StringRef Prefix) {
+  auto MD = getMetadata(LLVMContext::MD_section_prefix);
+  StringRef ExistingPrefix; // Empty by default.
+  if (MD != nullptr)
+    ExistingPrefix = cast<MDString>(MD->getOperand(1))->getString();
+
+  if (ExistingPrefix != Prefix) {
+    setSectionPrefix(Prefix);
+    return true;
+  }
+  return false;
+}
+
 std::optional<StringRef> GlobalObject::getSectionPrefix() const {
   if (MDNode *MD = getMetadata(LLVMContext::MD_section_prefix)) {
     [[maybe_unused]] StringRef MDName =
diff --git a/llvm/unittests/IR/CMakeLists.txt b/llvm/unittests/IR/CMakeLists.txt
index 8b7bd3997ea27..d62ce66ef9d34 100644
--- a/llvm/unittests/IR/CMakeLists.txt
+++ b/llvm/unittests/IR/CMakeLists.txt
@@ -28,6 +28,7 @@ add_llvm_unittest(IRTests
   DominatorTreeBatchUpdatesTest.cpp
   DroppedVariableStatsIRTest.cpp
   FunctionTest.cpp
+  GlobalObjectTest.cpp
   PassBuilderCallbacksTest.cpp
   IRBuilderTest.cpp
   InstructionsTest.cpp
diff --git a/llvm/unittests/IR/GlobalObjectTest.cpp b/llvm/unittests/IR/GlobalObjectTest.cpp
new file mode 100644
index 0000000000000..26949ae3a39fa
--- /dev/null
+++ b/llvm/unittests/IR/GlobalObjectTest.cpp
@@ -0,0 +1,76 @@
+//===- GlobalObjectTest.cpp - Global object unit tests --------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/IR/GlobalObject.h"
+#include "llvm-c/Core.h"
+#include "llvm/AsmParser/Parser.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+using namespace llvm;
+namespace {
+using testing::Eq;
+using testing::Optional;
+using testing::StrEq;
+
+static std::unique_ptr<Module> parseIR(LLVMContext &C, const char *IR) {
+  SMDiagnostic Err;
+  std::unique_ptr<Module> Mod = parseAssemblyString(IR, Err, C);
+  if (!Mod)
+    Err.print("GlobalObjectTests", errs());
+  return Mod;
+}
+
+static LLVMContext C;
+static std::unique_ptr<Module> M;
+
+class GlobalObjectTest : public testing::Test {
+public:
+  static void SetUpTestSuite() {
+    M = parseIR(C, R"(
+@foo = global i32 3, !section_prefix !0
+@bar = global i32 0
+
+!0 = !{!"section_prefix", !"hot"}
+)");
+  }
+};
+
+TEST_F(GlobalObjectTest, SectionPrefix) {
+  GlobalVariable *Foo = M->getGlobalVariable("foo");
+
+  // Initial section prefix is hot.
+  ASSERT_NE(Foo, nullptr);
+  ASSERT_THAT(Foo->getSectionPrefix(), Optional(StrEq("hot")));
+
+  // No actual update.
+  EXPECT_FALSE(Foo->updateSectionPrefix("hot"));
+
+  // Update prefix from hot to unlikely.
+  Foo->setSectionPrefix("unlikely");
+  EXPECT_THAT(Foo->getSectionPrefix(), Optional(StrEq("unlikely")));
+
+  // Update prefix to empty is the same as clear.
+  Foo->setSectionPrefix("");
+  EXPECT_THAT(Foo->getSectionPrefix(), Eq(std::nullopt));
+
+  GlobalVariable *Bar = M->getGlobalVariable("bar");
+
+  // Initial section prefix is empty.
+  ASSERT_NE(Bar, nullptr);
+  ASSERT_THAT(Bar->getSectionPrefix(), Eq(std::nullopt));
+
+  // No actual update.
+  EXPECT_FALSE(Bar->updateSectionPrefix(""));
+
+  // Update from empty to hot.
+  EXPECT_TRUE(Bar->updateSectionPrefix("hot"));
+  EXPECT_THAT(Bar->getSectionPrefix(), Optional(StrEq("hot")));
+}
+} // namespace

mingmingl-llvm

The title and summary are both mechanical. Maybe add a 1 sentence description of why this change is needed?

Sure! The updated PR description points to the use case.

Also clicked 'ready for review' to make this non-draft. Forgot to do this when I requested reviews before.

llvm/include/llvm/IR/GlobalObject.h

mingmingl-llvm · 2025-09-15T22:07:02Z

llvm/unittests/IR/GlobalObjectTest.cpp

+  Foo->setSectionPrefix("unlikely");
+  EXPECT_THAT(Foo->getSectionPrefix(), Optional(StrEq("unlikely")));
+
+  // Update prefix to empty is the same as clear.


mingmingl-llvm · 2025-09-15T22:21:03Z

llvm/unittests/IR/GlobalObjectTest.cpp

+//===----------------------------------------------------------------------===//
+
+#include "llvm/IR/GlobalObject.h"
+#include "llvm-c/Core.h"


The headers are copied from FunctionTest.cpp. I removed the unused ones now.

llvm/unittests/IR/GlobalObjectTest.cpp

mingmingl-llvm · 2025-09-15T22:34:40Z

llvm/lib/IR/Globals.cpp

 }

+bool GlobalObject::updateSectionPrefix(StringRef Prefix) {
+  auto MD = getMetadata(LLVMContext::MD_section_prefix);


Use getSectionPrefix instead? It does some assertion checking that could be useful.

done.

does it make sense to just make this the default behavior of setSectionPrefix?

Implementation wise, the update method currently calls the set method (the other way around). Technically, no existing caller of 'set' depends on the behavior that 'set' method will overwrite, so either way (with a brief comment on the function decl in header) is fine imo. Doing it the current way will avoid affecting (fwiw) existing user in codegen prepare to set function section prefix though.

snehasish · 2025-09-16T02:02:34Z

Did you push your changes yet? I don't see the updated diff.

mingmingl-llvm · 2025-09-16T02:56:55Z

Did you push your changes yet? I don't see the updated diff.

Sorry! This is pushed now.

snehasish

Lgtm

snehasish · 2025-09-16T03:00:12Z

llvm/unittests/IR/GlobalObjectTest.cpp

+  EXPECT_TRUE(Bar->updateSectionPrefix("hot"));
+  EXPECT_THAT(Bar->getSectionPrefix(), Optional(StrEq("hot")));
+
+  // Teset that update method returns true and section prefix is cleared.


…alue

teresajohnson

lgtm but comment needs a slight fix as noted below.

teresajohnson · 2025-09-16T18:08:52Z

llvm/include/llvm/IR/GlobalObject.h


-  /// Set the section prefix for this global object.
-  LLVM_ABI void setSectionPrefix(StringRef Prefix);
+  /// If existing prefix is different from \p Prefix is different, set it to


redundant "different"

thanks for the catch! fixed it.

mingmingl-llvm · 2025-09-16T19:01:15Z

In both windows and linux, the failed tests are irrelevant. Going to merge this change.

  ********************
  Failed Tests (2):
    LLVM :: CodeGen/X86/memcmp-pgso-x32.ll
    LLVM :: CodeGen/X86/memcmp-pgso.ll

llvm-ci · 2025-09-16T19:16:06Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-sie-ubuntu-fast running on sie-linux-worker while building llvm at step 6 "test-build-unified-tree-check-all".

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