Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion llvm/include/llvm/IR/GlobalObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
17 changes: 17 additions & 0 deletions llvm/lib/IR/Globals.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@mingmingl-llvm mingmingl-llvm Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

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 =
Expand Down
1 change: 1 addition & 0 deletions llvm/unittests/IR/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ add_llvm_unittest(IRTests
DominatorTreeBatchUpdatesTest.cpp
DroppedVariableStatsIRTest.cpp
FunctionTest.cpp
GlobalObjectTest.cpp
PassBuilderCallbacksTest.cpp
IRBuilderTest.cpp
InstructionsTest.cpp
Expand Down
76 changes: 76 additions & 0 deletions llvm/unittests/IR/GlobalObjectTest.cpp
Original file line number Diff line number Diff line change
@@ -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"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what this header is used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

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
Loading