Re-apply "[NFCI][Globals] In GlobalObjects::setSectionPrefix, do conditional update if existing prefix is not equivalent to the new one. Returns whether prefix changed." #159161
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ectionPrefix to handle empty strings
|
@llvm/pr-subscribers-llvm-ir Author: Mingming Liu (mingmingl-llvm) ChangesThis is a reland of #158460 Test failures are gone once I undo the changes in codegenprepare. Full diff: https://github.com/llvm/llvm-project/pull/159161.diff 7 Files Affected:
diff --git a/llvm/include/llvm/IR/GlobalObject.h b/llvm/include/llvm/IR/GlobalObject.h
index 08a02b42bdc14..e273387807cf6 100644
--- a/llvm/include/llvm/IR/GlobalObject.h
+++ b/llvm/include/llvm/IR/GlobalObject.h
@@ -121,8 +121,10 @@ class GlobalObject : public GlobalValue {
/// appropriate default object file section.
LLVM_ABI void setSection(StringRef S);
- /// Set the section prefix for this global object.
- LLVM_ABI void setSectionPrefix(StringRef Prefix);
+ /// If existing prefix is different from \p Prefix, set it to \p Prefix. If \p
+ /// Prefix is empty, the set clears the existing metadata. Returns true if
+ /// section prefix changed and false otherwise.
+ LLVM_ABI bool setSectionPrefix(StringRef Prefix);
/// Get the section prefix for this global object.
LLVM_ABI std::optional<StringRef> getSectionPrefix() const;
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 9db4c9e5e2807..a190f0dac1379 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -583,23 +583,23 @@ bool CodeGenPrepare::_run(Function &F) {
// if requested.
if (BBSectionsGuidedSectionPrefix && BBSectionsProfileReader &&
BBSectionsProfileReader->isFunctionHot(F.getName())) {
- F.setSectionPrefix("hot");
+ (void)F.setSectionPrefix("hot");
} else if (ProfileGuidedSectionPrefix) {
// The hot attribute overwrites profile count based hotness while profile
// counts based hotness overwrite the cold attribute.
// This is a conservative behabvior.
if (F.hasFnAttribute(Attribute::Hot) ||
PSI->isFunctionHotInCallGraph(&F, *BFI))
- F.setSectionPrefix("hot");
+ (void)F.setSectionPrefix("hot");
// If PSI shows this function is not hot, we will placed the function
// into unlikely section if (1) PSI shows this is a cold function, or
// (2) the function has a attribute of cold.
else if (PSI->isFunctionColdInCallGraph(&F, *BFI) ||
F.hasFnAttribute(Attribute::Cold))
- F.setSectionPrefix("unlikely");
+ (void)F.setSectionPrefix("unlikely");
else if (ProfileUnknownInSpecialSection && PSI->hasPartialSampleProfile() &&
PSI->isFunctionHotnessUnknown(F))
- F.setSectionPrefix("unknown");
+ (void)F.setSectionPrefix("unknown");
}
/// This optimization identifies DIV instructions that can be
diff --git a/llvm/lib/CodeGen/StaticDataAnnotator.cpp b/llvm/lib/CodeGen/StaticDataAnnotator.cpp
index 2d9b489a80acb..53a9ab4dbda02 100644
--- a/llvm/lib/CodeGen/StaticDataAnnotator.cpp
+++ b/llvm/lib/CodeGen/StaticDataAnnotator.cpp
@@ -91,8 +91,7 @@ bool StaticDataAnnotator::runOnModule(Module &M) {
if (SectionPrefix.empty())
continue;
- GV.setSectionPrefix(SectionPrefix);
- Changed = true;
+ Changed |= GV.setSectionPrefix(SectionPrefix);
}
return Changed;
diff --git a/llvm/lib/IR/Globals.cpp b/llvm/lib/IR/Globals.cpp
index 11d33e262fecb..1a7a5c5fbad6b 100644
--- a/llvm/lib/IR/Globals.cpp
+++ b/llvm/lib/IR/Globals.cpp
@@ -288,10 +288,22 @@ void GlobalObject::setSection(StringRef S) {
setGlobalObjectFlag(HasSectionHashEntryBit, !S.empty());
}
-void GlobalObject::setSectionPrefix(StringRef Prefix) {
+bool GlobalObject::setSectionPrefix(StringRef Prefix) {
+ StringRef ExistingPrefix;
+ if (std::optional<StringRef> MaybePrefix = getSectionPrefix())
+ ExistingPrefix = *MaybePrefix;
+
+ if (ExistingPrefix == Prefix)
+ return false;
+
+ if (Prefix.empty()) {
+ setMetadata(LLVMContext::MD_section_prefix, nullptr);
+ return true;
+ }
MDBuilder MDB(getContext());
setMetadata(LLVMContext::MD_section_prefix,
MDB.createGlobalObjectSectionPrefix(Prefix));
+ return true;
}
std::optional<StringRef> GlobalObject::getSectionPrefix() const {
diff --git a/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp b/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
index ecb2f2dbc552b..c86092bd51eda 100644
--- a/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
@@ -848,13 +848,12 @@ bool MemProfUsePass::annotateGlobalVariables(
// So we just print out the static data section prefix in LLVM_DEBUG.
if (Record && Record->AccessCount > 0) {
++NumOfMemProfHotGlobalVars;
- GVar.setSectionPrefix("hot");
- Changed = true;
+ Changed |= GVar.setSectionPrefix("hot");
LLVM_DEBUG(dbgs() << "Global variable " << Name
<< " is annotated as hot\n");
} else if (DataAccessProf->isKnownColdSymbol(Name)) {
++NumOfMemProfColdGlobalVars;
- GVar.setSectionPrefix("unlikely");
+ Changed |= GVar.setSectionPrefix("unlikely");
Changed = true;
LLVM_DEBUG(dbgs() << "Global variable " << Name
<< " is annotated as unlikely\n");
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..0e16d01e759de
--- /dev/null
+++ b/llvm/unittests/IR/GlobalObjectTest.cpp
@@ -0,0 +1,80 @@
+//===- 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/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")));
+
+ // Test that set method returns false since existing section prefix is hot.
+ EXPECT_FALSE(Foo->setSectionPrefix("hot"));
+
+ // Set prefix from hot to unlikely.
+ Foo->setSectionPrefix("unlikely");
+ EXPECT_THAT(Foo->getSectionPrefix(), Optional(StrEq("unlikely")));
+
+ // Set prefix to empty is the same as clear.
+ Foo->setSectionPrefix("");
+ // Test that section prefix is cleared.
+ 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));
+
+ // Test that set method returns false since Bar doesn't have prefix metadata.
+ EXPECT_FALSE(Bar->setSectionPrefix(""));
+
+ // Set from empty to hot.
+ EXPECT_TRUE(Bar->setSectionPrefix("hot"));
+ EXPECT_THAT(Bar->getSectionPrefix(), Optional(StrEq("hot")));
+
+ // Test that set method returns true and section prefix is cleared.
+ EXPECT_TRUE(Bar->setSectionPrefix(""));
+ EXPECT_THAT(Bar->getSectionPrefix(), Eq(std::nullopt));
+}
+} // namespace
|
|
@llvm/pr-subscribers-llvm-transforms Author: Mingming Liu (mingmingl-llvm) ChangesThis is a reland of #158460 Test failures are gone once I undo the changes in codegenprepare. Full diff: https://github.com/llvm/llvm-project/pull/159161.diff 7 Files Affected:
diff --git a/llvm/include/llvm/IR/GlobalObject.h b/llvm/include/llvm/IR/GlobalObject.h
index 08a02b42bdc14..e273387807cf6 100644
--- a/llvm/include/llvm/IR/GlobalObject.h
+++ b/llvm/include/llvm/IR/GlobalObject.h
@@ -121,8 +121,10 @@ class GlobalObject : public GlobalValue {
/// appropriate default object file section.
LLVM_ABI void setSection(StringRef S);
- /// Set the section prefix for this global object.
- LLVM_ABI void setSectionPrefix(StringRef Prefix);
+ /// If existing prefix is different from \p Prefix, set it to \p Prefix. If \p
+ /// Prefix is empty, the set clears the existing metadata. Returns true if
+ /// section prefix changed and false otherwise.
+ LLVM_ABI bool setSectionPrefix(StringRef Prefix);
/// Get the section prefix for this global object.
LLVM_ABI std::optional<StringRef> getSectionPrefix() const;
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 9db4c9e5e2807..a190f0dac1379 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -583,23 +583,23 @@ bool CodeGenPrepare::_run(Function &F) {
// if requested.
if (BBSectionsGuidedSectionPrefix && BBSectionsProfileReader &&
BBSectionsProfileReader->isFunctionHot(F.getName())) {
- F.setSectionPrefix("hot");
+ (void)F.setSectionPrefix("hot");
} else if (ProfileGuidedSectionPrefix) {
// The hot attribute overwrites profile count based hotness while profile
// counts based hotness overwrite the cold attribute.
// This is a conservative behabvior.
if (F.hasFnAttribute(Attribute::Hot) ||
PSI->isFunctionHotInCallGraph(&F, *BFI))
- F.setSectionPrefix("hot");
+ (void)F.setSectionPrefix("hot");
// If PSI shows this function is not hot, we will placed the function
// into unlikely section if (1) PSI shows this is a cold function, or
// (2) the function has a attribute of cold.
else if (PSI->isFunctionColdInCallGraph(&F, *BFI) ||
F.hasFnAttribute(Attribute::Cold))
- F.setSectionPrefix("unlikely");
+ (void)F.setSectionPrefix("unlikely");
else if (ProfileUnknownInSpecialSection && PSI->hasPartialSampleProfile() &&
PSI->isFunctionHotnessUnknown(F))
- F.setSectionPrefix("unknown");
+ (void)F.setSectionPrefix("unknown");
}
/// This optimization identifies DIV instructions that can be
diff --git a/llvm/lib/CodeGen/StaticDataAnnotator.cpp b/llvm/lib/CodeGen/StaticDataAnnotator.cpp
index 2d9b489a80acb..53a9ab4dbda02 100644
--- a/llvm/lib/CodeGen/StaticDataAnnotator.cpp
+++ b/llvm/lib/CodeGen/StaticDataAnnotator.cpp
@@ -91,8 +91,7 @@ bool StaticDataAnnotator::runOnModule(Module &M) {
if (SectionPrefix.empty())
continue;
- GV.setSectionPrefix(SectionPrefix);
- Changed = true;
+ Changed |= GV.setSectionPrefix(SectionPrefix);
}
return Changed;
diff --git a/llvm/lib/IR/Globals.cpp b/llvm/lib/IR/Globals.cpp
index 11d33e262fecb..1a7a5c5fbad6b 100644
--- a/llvm/lib/IR/Globals.cpp
+++ b/llvm/lib/IR/Globals.cpp
@@ -288,10 +288,22 @@ void GlobalObject::setSection(StringRef S) {
setGlobalObjectFlag(HasSectionHashEntryBit, !S.empty());
}
-void GlobalObject::setSectionPrefix(StringRef Prefix) {
+bool GlobalObject::setSectionPrefix(StringRef Prefix) {
+ StringRef ExistingPrefix;
+ if (std::optional<StringRef> MaybePrefix = getSectionPrefix())
+ ExistingPrefix = *MaybePrefix;
+
+ if (ExistingPrefix == Prefix)
+ return false;
+
+ if (Prefix.empty()) {
+ setMetadata(LLVMContext::MD_section_prefix, nullptr);
+ return true;
+ }
MDBuilder MDB(getContext());
setMetadata(LLVMContext::MD_section_prefix,
MDB.createGlobalObjectSectionPrefix(Prefix));
+ return true;
}
std::optional<StringRef> GlobalObject::getSectionPrefix() const {
diff --git a/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp b/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
index ecb2f2dbc552b..c86092bd51eda 100644
--- a/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
@@ -848,13 +848,12 @@ bool MemProfUsePass::annotateGlobalVariables(
// So we just print out the static data section prefix in LLVM_DEBUG.
if (Record && Record->AccessCount > 0) {
++NumOfMemProfHotGlobalVars;
- GVar.setSectionPrefix("hot");
- Changed = true;
+ Changed |= GVar.setSectionPrefix("hot");
LLVM_DEBUG(dbgs() << "Global variable " << Name
<< " is annotated as hot\n");
} else if (DataAccessProf->isKnownColdSymbol(Name)) {
++NumOfMemProfColdGlobalVars;
- GVar.setSectionPrefix("unlikely");
+ Changed |= GVar.setSectionPrefix("unlikely");
Changed = true;
LLVM_DEBUG(dbgs() << "Global variable " << Name
<< " is annotated as unlikely\n");
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..0e16d01e759de
--- /dev/null
+++ b/llvm/unittests/IR/GlobalObjectTest.cpp
@@ -0,0 +1,80 @@
+//===- 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/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")));
+
+ // Test that set method returns false since existing section prefix is hot.
+ EXPECT_FALSE(Foo->setSectionPrefix("hot"));
+
+ // Set prefix from hot to unlikely.
+ Foo->setSectionPrefix("unlikely");
+ EXPECT_THAT(Foo->getSectionPrefix(), Optional(StrEq("unlikely")));
+
+ // Set prefix to empty is the same as clear.
+ Foo->setSectionPrefix("");
+ // Test that section prefix is cleared.
+ 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));
+
+ // Test that set method returns false since Bar doesn't have prefix metadata.
+ EXPECT_FALSE(Bar->setSectionPrefix(""));
+
+ // Set from empty to hot.
+ EXPECT_TRUE(Bar->setSectionPrefix("hot"));
+ EXPECT_THAT(Bar->getSectionPrefix(), Optional(StrEq("hot")));
+
+ // Test that set method returns true and section prefix is cleared.
+ EXPECT_TRUE(Bar->setSectionPrefix(""));
+ EXPECT_THAT(Bar->getSectionPrefix(), Eq(std::nullopt));
+}
+} // namespace
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/31261 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/16145 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/12909 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a reland of #158460
Test failures are gone once I undo the changes in codegenprepare.