[StructuralHash] Global Variable #118412
Merged
[StructuralHash] Global Variable #118412
+244
−13
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
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
cc @nocchijiang |
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-backend-aarch64 Author: Kyungwoo Lee (kyulee-com) ChangesThis update enhances the implementation of structural hashing for global variables, using their initial contents. Private global variables or constants are often used for metadata, where their names are not unique. This can lead to the creation of different hash results although they could be merged by the linker as they are effectively identical.
This change significantly improves size reduction by an additional 1% on the LLD binary when the global function outliner and merger are enabled together. As discussed in the RFC, if we disable or relocate the LOH pass, the size impact could increase to 4%. Full diff: https://github.com/llvm/llvm-project/pull/118412.diff 5 Files Affected:
diff --git a/llvm/include/llvm/IR/StructuralHash.h b/llvm/include/llvm/IR/StructuralHash.h
index 071575137ff572..514dd6f174b903 100644
--- a/llvm/include/llvm/IR/StructuralHash.h
+++ b/llvm/include/llvm/IR/StructuralHash.h
@@ -31,6 +31,9 @@ class Module;
/// to true includes instruction and operand type information.
stable_hash StructuralHash(const Function &F, bool DetailedHash = false);
+/// Returns a hash of the global variable \p G.
+stable_hash StructuralHash(const GlobalVariable &G);
+
/// Returns a hash of the module \p M by hashing all functions and global
/// variables contained within. \param M The module to hash. \param DetailedHash
/// Whether or not to encode additional information in the function hashes that
diff --git a/llvm/lib/CodeGen/MachineStableHash.cpp b/llvm/lib/CodeGen/MachineStableHash.cpp
index facda7a59e2f86..09a81cb318ecb7 100644
--- a/llvm/lib/CodeGen/MachineStableHash.cpp
+++ b/llvm/lib/CodeGen/MachineStableHash.cpp
@@ -27,6 +27,8 @@
#include "llvm/CodeGen/Register.h"
#include "llvm/Config/llvm-config.h"
#include "llvm/IR/Constants.h"
+#include "llvm/IR/GlobalVariable.h"
+#include "llvm/IR/StructuralHash.h"
#include "llvm/MC/MCSymbol.h"
#include "llvm/Support/Alignment.h"
#include "llvm/Support/ErrorHandling.h"
@@ -97,9 +99,14 @@ stable_hash llvm::stableHashValue(const MachineOperand &MO) {
++StableHashBailingGlobalAddress;
return 0;
}
- auto Name = GV->getName();
- return stable_hash_combine(MO.getType(), MO.getTargetFlags(),
- stable_hash_name(Name), MO.getOffset());
+ stable_hash GVHash = 0;
+ if (auto *GVar = dyn_cast<GlobalVariable>(GV))
+ GVHash = StructuralHash(*GVar);
+ if (!GVHash)
+ GVHash = stable_hash_name(GV->getName());
+
+ return stable_hash_combine(MO.getType(), MO.getTargetFlags(), GVHash,
+ MO.getOffset());
}
case MachineOperand::MO_TargetIndex: {
diff --git a/llvm/lib/IR/StructuralHash.cpp b/llvm/lib/IR/StructuralHash.cpp
index ccc534a8904191..de883f81a8e4a5 100644
--- a/llvm/lib/IR/StructuralHash.cpp
+++ b/llvm/lib/IR/StructuralHash.cpp
@@ -46,7 +46,7 @@ class StructuralHashImpl {
/// Assign a unique ID to each Value in the order they are first seen.
DenseMap<const Value *, int> ValueToId;
- stable_hash hashType(Type *ValueType) {
+ static stable_hash hashType(Type *ValueType) {
SmallVector<stable_hash> Hashes;
Hashes.emplace_back(ValueType->getTypeID());
if (ValueType->isIntegerTy())
@@ -65,7 +65,7 @@ class StructuralHashImpl {
}
}
- stable_hash hashAPInt(const APInt &I) {
+ static stable_hash hashAPInt(const APInt &I) {
SmallVector<stable_hash> Hashes;
Hashes.emplace_back(I.getBitWidth());
auto RawVals = ArrayRef<uint64_t>(I.getRawData(), I.getNumWords());
@@ -73,11 +73,36 @@ class StructuralHashImpl {
return stable_hash_combine(Hashes);
}
- stable_hash hashAPFloat(const APFloat &F) {
+ static stable_hash hashAPFloat(const APFloat &F) {
return hashAPInt(F.bitcastToAPInt());
}
- stable_hash hashGlobalValue(const GlobalValue *GV) {
+ static stable_hash hashGlobalVariable(const GlobalVariable &GVar) {
+ if (!GVar.hasInitializer())
+ return hashGlobalValue(&GVar);
+
+ // Hash the contents of a string.
+ if (GVar.getName().starts_with(".str"))
+ return hashConstant(GVar.getInitializer());
+
+ // Hash structural contents of Objective-C metadata in specific sections.
+ // This can be extended to other metadata if needed.
+ static constexpr const char *SectionNames[] = {
+ "__cfstring", "__cstring", "__objc_classrefs",
+ "__objc_methname", "__objc_selrefs",
+ };
+ if (GVar.hasSection()) {
+ StringRef SectionName = GVar.getSection();
+ for (const char *Name : SectionNames) {
+ if (SectionName.contains(Name))
+ return hashConstant(GVar.getInitializer());
+ }
+ }
+
+ return hashGlobalValue(&GVar);
+ }
+
+ static stable_hash hashGlobalValue(const GlobalValue *GV) {
if (!GV->hasName())
return 0;
return stable_hash_name(GV->getName());
@@ -87,7 +112,7 @@ class StructuralHashImpl {
// FunctionComparator::cmpConstants() in FunctionComparator.cpp, but here
// we're interested in computing a hash rather than comparing two Constants.
// Some of the logic is simplified, e.g, we don't expand GEPOperator.
- stable_hash hashConstant(Constant *C) {
+ static stable_hash hashConstant(const Constant *C) {
SmallVector<stable_hash> Hashes;
Type *Ty = C->getType();
@@ -98,14 +123,21 @@ class StructuralHashImpl {
return stable_hash_combine(Hashes);
}
+ if (auto *GVar = dyn_cast<GlobalVariable>(C)) {
+ Hashes.emplace_back(hashGlobalVariable(*GVar));
+ return stable_hash_combine(Hashes);
+ }
+
if (auto *G = dyn_cast<GlobalValue>(C)) {
Hashes.emplace_back(hashGlobalValue(G));
return stable_hash_combine(Hashes);
}
if (const auto *Seq = dyn_cast<ConstantDataSequential>(C)) {
- Hashes.emplace_back(xxh3_64bits(Seq->getRawDataValues()));
- return stable_hash_combine(Hashes);
+ if (Seq->isString()) {
+ Hashes.emplace_back(stable_hash_name(Seq->getAsString()));
+ return stable_hash_combine(Hashes);
+ }
}
switch (C->getValueID()) {
@@ -266,6 +298,7 @@ class StructuralHashImpl {
Hashes.emplace_back(Hash);
Hashes.emplace_back(GlobalHeaderHash);
Hashes.emplace_back(GV.getValueType()->getTypeID());
+ Hashes.emplace_back(hashGlobalVariable(GV));
// Update the combined hash in place.
Hash = stable_hash_combine(Hashes);
@@ -297,6 +330,10 @@ stable_hash llvm::StructuralHash(const Function &F, bool DetailedHash) {
return H.getHash();
}
+stable_hash llvm::StructuralHash(const GlobalVariable &GVar) {
+ return StructuralHashImpl::hashGlobalVariable(GVar);
+}
+
stable_hash llvm::StructuralHash(const Module &M, bool DetailedHash) {
StructuralHashImpl H(DetailedHash);
H.update(M);
diff --git a/llvm/test/CodeGen/AArch64/cgdata-merge-gvar.ll b/llvm/test/CodeGen/AArch64/cgdata-merge-gvar.ll
new file mode 100644
index 00000000000000..f1f5209abe3507
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/cgdata-merge-gvar.ll
@@ -0,0 +1,91 @@
+; This test verifies that global variables are hashed based on their initial contents,
+; allowing them to be merged even if they appear different due to their names.
+; Now they become identical functions that can be merged without creating a paramter.
+
+; RUN: rm -rf %t && split-file %s %t
+
+; RUN: llc -mtriple=arm64-apple-darwin -enable-global-merge-func=true -global-merging-skip-no-params=false < %t/string.ll | FileCheck %s
+; RUN: llc -mtriple=arm64-apple-darwin -enable-global-merge-func=true -global-merging-skip-no-params=false < %t/ns-const.ll | FileCheck %s
+; RUN: llc -mtriple=arm64-apple-darwin -enable-global-merge-func=true -global-merging-skip-no-params=false < %t/objc-ref.ll | FileCheck %s
+
+; CHECK: _f1.Tgm
+; CHECK: _f2.Tgm
+
+;--- string.ll
+
+@.str = private unnamed_addr constant [6 x i8] c"hello\00", align 1
+@.str.1 = private unnamed_addr constant [6 x i8] c"hello\00", align 1
+
+declare noundef i32 @goo(ptr noundef)
+
+define i32 @f1() {
+entry:
+ %call = tail call noundef i32 @goo(ptr noundef nonnull @.str)
+ %add = add nsw i32 %call, 1
+ ret i32 %add
+}
+
+define i32 @f2() {
+entry:
+ %call = tail call noundef i32 @goo(ptr noundef nonnull @.str.1)
+ %add = add nsw i32 %call, 1
+ ret i32 %add
+}
+
+;--- ns-const.ll
+
+%struct.__NSConstantString_tag = type { ptr, i32, ptr, i64 }
+@__CFConstantStringClassReference = external global [0 x i32]
+@.str.2 = private unnamed_addr constant [9 x i8] c"cfstring\00", section "__TEXT,__cstring,cstring_literals", align 1
+@_unnamed_cfstring_ = private global %struct.__NSConstantString_tag { ptr @__CFConstantStringClassReference, i32 1992, ptr @.str.2, i64 8 }, section "__DATA,__cfstring", align 8
+
+@.str.3 = private unnamed_addr constant [9 x i8] c"cfstring\00", section "__TEXT,__cstring,cstring_literals", align 1
+@_unnamed_cfstring_.2 = private global %struct.__NSConstantString_tag { ptr @__CFConstantStringClassReference, i32 1992, ptr @.str.3, i64 8 }, section "__DATA,__cfstring", align 8
+
+declare noundef i32 @hoo(ptr noundef)
+
+define i32 @f1() {
+entry:
+ %call = tail call i32 @hoo(ptr noundef nonnull @_unnamed_cfstring_)
+ %add = sub nsw i32 %call, 1
+ ret i32 %add
+}
+
+define i32 @f2() {
+entry:
+ %call = tail call i32 @hoo(ptr noundef nonnull @_unnamed_cfstring_.2)
+ %add = sub nsw i32 %call, 1
+ ret i32 %add
+}
+
+;--- objc-ref.ll
+
+%struct._class_t = type { ptr, ptr, ptr, ptr, ptr }
+
+@"OBJC_CLASS_$_MyClass" = external global %struct._class_t
+@"OBJC_CLASSLIST_REFERENCES_$_" = internal global ptr @"OBJC_CLASS_$_MyClass", section "__DATA,__objc_classrefs,regular,no_dead_strip", align 8
+@"OBJC_CLASSLIST_REFERENCES_$_.1" = internal global ptr @"OBJC_CLASS_$_MyClass", section "__DATA,__objc_classrefs,regular,no_dead_strip", align 8
+
+@OBJC_METH_VAR_NAME_ = private unnamed_addr constant [6 x i8] c"hello\00", section "__TEXT,__objc_methname,cstring_literals", align 1
+@OBJC_METH_VAR_NAME_.1 = private unnamed_addr constant [6 x i8] c"hello\00", section "__TEXT,__objc_methname,cstring_literals", align 1
+
+@OBJC_SELECTOR_REFERENCES_ = internal externally_initialized global ptr @OBJC_METH_VAR_NAME_, section "__DATA,__objc_selrefs,literal_pointers,no_dead_strip", align 8
+@OBJC_SELECTOR_REFERENCES_.1 = internal externally_initialized global ptr @OBJC_METH_VAR_NAME_.1, section "__DATA,__objc_selrefs,literal_pointers,no_dead_strip", align 8
+
+define i32 @f1() {
+entry:
+ %0 = load ptr, ptr @"OBJC_CLASSLIST_REFERENCES_$_", align 8
+ %1 = load ptr, ptr @OBJC_SELECTOR_REFERENCES_, align 8
+ %call = tail call noundef i32 @objc_msgSend(ptr noundef %0, ptr noundef %1)
+ ret i32 %call
+}
+
+declare ptr @objc_msgSend(ptr, ptr, ...)
+
+define i32 @f2() {
+entry:
+ %0 = load ptr, ptr @"OBJC_CLASSLIST_REFERENCES_$_.1", align 8
+ %1 = load ptr, ptr @OBJC_SELECTOR_REFERENCES_.1, align 8
+ %call = tail call noundef i32 @objc_msgSend(ptr noundef %0, ptr noundef %1)
+ ret i32 %call
+}
diff --git a/llvm/test/CodeGen/AArch64/cgdata-outline-gvar.ll b/llvm/test/CodeGen/AArch64/cgdata-outline-gvar.ll
new file mode 100644
index 00000000000000..447928dfa07245
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/cgdata-outline-gvar.ll
@@ -0,0 +1,52 @@
+; This test verifies that global variables are hashed based on their initial contents,
+; allowing them to be outlined even if they appear different due to their names.
+
+; RUN: split-file %s %t
+
+; Check if the outlined function is created locally.
+; RUN: llc -mtriple=arm64-apple-darwin -enable-machine-outliner -codegen-data-generate=true -aarch64-enable-collect-loh=false -filetype=obj %t/local-two.ll -o %t_write_base
+; RUN: llvm-objdump -d %t_write_base | FileCheck %s
+
+; RUN: llvm-cgdata --merge %t_write_base -o %t_cgdata_base
+
+; Read the cgdata in the machine outliner for optimistically outlining in local-one.ll.
+; RUN: llc -mtriple=arm64-apple-darwin -enable-machine-outliner -codegen-data-use-path=%t_cgdata_base -aarch64-enable-collect-loh=false -append-content-hash-outlined-name=false -filetype=obj %t/local-one.ll -o %t_read_base
+; RUN: llvm-objdump -d %t_read_base | FileCheck %s
+
+; The names of globals `.str` and `.str.4` are different, but their initial contents are identical.
+; The outlined function now starts with a reference to that global ("hello\00").
+; CHECK: _OUTLINED_FUNCTION_{{.*}}:
+; CHECK-NEXT: adrp x1
+; CHECK-NEXT: add x1, x1
+; CHECK-NEXT: mov w2
+; CHECK-NEXT: mov w3
+; CHECK-NEXT: mov w4
+; CHECK-NEXT: b
+
+;--- local-two.ll
+@.str = private unnamed_addr constant [6 x i8] c"hello\00", align 1
+@.str.1 = private unnamed_addr constant [3 x i8] c"f1\00", align 1
+@.str.2 = private unnamed_addr constant [3 x i8] c"f2\00", align 1
+
+declare noundef i32 @goo(ptr noundef, ptr noundef, i32, i32, i32)
+define i32 @f1() minsize {
+entry:
+ %call = tail call noundef i32 @goo(ptr noundef nonnull @.str.1, ptr noundef nonnull @.str, i32 1, i32 2, i32 3)
+ ret i32 %call
+}
+define i32 @f2() minsize {
+entry:
+ %call = tail call noundef i32 @goo(ptr noundef nonnull @.str.2, ptr noundef nonnull @.str, i32 1, i32 2, i32 3)
+ ret i32 %call
+}
+
+;--- local-one.ll
+@.str.3 = private unnamed_addr constant [3 x i8] c"f3\00", align 1
+@.str.4 = private unnamed_addr constant [6 x i8] c"hello\00", align 1
+
+declare noundef i32 @goo(ptr noundef, ptr noundef, i32, i32, i32)
+define i32 @f1() minsize {
+entry:
+ %call = tail call noundef i32 @goo(ptr noundef nonnull @.str.3, ptr noundef nonnull @.str.4, i32 1, i32 2, i32 3)
+ ret i32 %call
+}
|
| @@ -0,0 +1,91 @@ | |||
| ; This test verifies that global variables are hashed based on their initial contents, | |||
| ; allowing them to be merged even if they appear different due to their names. | |||
| ; Now they become identical functions that can be merged without creating a paramter. | |||
There was a problem hiding this comment.
(a drive-by comment) nit: s/paramter/parameter
llvm/lib/IR/StructuralHash.cpp
Outdated
|
|
||
| // Hash the contents of a string. | ||
| if (GVar.getName().starts_with(".str")) | ||
| return hashConstant(GVar.getInitializer()); |
There was a problem hiding this comment.
.str is the prefix of almost all (if not all) string literals in the LLVM IR that I've seen. I'm wondering if there is a place to verify it's true (e.g., .str prefix means it's a string literal, and a string literal starts with .str).
Ask for my information since I'm making the same assumption for one recent prototype.
There was a problem hiding this comment.
I only use hasInitializer() to determine if the global variable (beginning with .str) has an initializer. While I don't specifically verify if it's a string literal, this approach should suffice for most scenarios. It's still safe for our purposes, even if it's not a string literal. The only concern is avoiding an initializer that recursively references the current global variable itself. I assume that the cases filtered out here and below do not include such instances.
ellishg
reviewed
Dec 3, 2024
llvm/lib/IR/StructuralHash.cpp
Outdated
Comment on lines
96
to
99
| for (const char *Name : SectionNames) { | ||
| if (SectionName.contains(Name)) | ||
| return hashConstant(GVar.getInitializer()); | ||
| } |
There was a problem hiding this comment.
Suggested change
| for (const char *Name : SectionNames) { | |
| if (SectionName.contains(Name)) | |
| return hashConstant(GVar.getInitializer()); | |
| } | |
| for (const char *Name : SectionNames) | |
| if (SectionName.contains(Name)) | |
| return hashConstant(GVar.getInitializer()); |
| ;--- string.ll | ||
|
|
||
| @.str = private unnamed_addr constant [6 x i8] c"hello\00", align 1 | ||
| @.str.1 = private unnamed_addr constant [6 x i8] c"hello\00", align 1 |
There was a problem hiding this comment.
Can you add another global string with a different value to ensure it isn't merged?
There was a problem hiding this comment.
Added a failing case.
Comment on lines
7
to
9
| ; RUN: llc -mtriple=arm64-apple-darwin -enable-global-merge-func=true -global-merging-skip-no-params=false < %t/string.ll | FileCheck %s | ||
| ; RUN: llc -mtriple=arm64-apple-darwin -enable-global-merge-func=true -global-merging-skip-no-params=false < %t/ns-const.ll | FileCheck %s | ||
| ; RUN: llc -mtriple=arm64-apple-darwin -enable-global-merge-func=true -global-merging-skip-no-params=false < %t/objc-ref.ll | FileCheck %s |
There was a problem hiding this comment.
These seem like three separate tests that don't need to share any data (except that their CHECK lines happen to be the same). Can we separate them into their own files?
There was a problem hiding this comment.
Separated the test cases into their own files.
| ; RUN: llvm-objdump -d %t_read_base | FileCheck %s | ||
|
|
||
| ; The names of globals `.str` and `.str.4` are different, but their initial contents are identical. | ||
| ; The outlined function now starts with a reference to that global ("hello\00"). |
There was a problem hiding this comment.
I'm a bit confused. Where is the reference to the "hello\00" string in this outlined function?
There was a problem hiding this comment.
Restructured the test cases.
llvm/lib/IR/StructuralHash.cpp
Outdated
| return hashGlobalValue(&GVar); | ||
|
|
||
| // Hash the contents of a string. | ||
| if (GVar.getName().starts_with(".str")) |
There was a problem hiding this comment.
This seems like a weird way to detect if a GV is a string. Is there a better way? Could we check if the type if [* x i8] or something?
There was a problem hiding this comment.
Added a string condition, using ConstantDataSequential.
nocchijiang
reviewed
Dec 3, 2024
| if (auto *G = dyn_cast<GlobalValue>(C)) { | ||
| Hashes.emplace_back(hashGlobalValue(G)); | ||
| return stable_hash_combine(Hashes); | ||
| } | ||
|
|
||
| if (const auto *Seq = dyn_cast<ConstantDataSequential>(C)) { | ||
| Hashes.emplace_back(xxh3_64bits(Seq->getRawDataValues())); | ||
| return stable_hash_combine(Hashes); | ||
| if (Seq->isString()) { |
There was a problem hiding this comment.
What about other ConstantDataSequentials which are not string?
There was a problem hiding this comment.
If you see below in the switch statement, ConstantArrayVal or ConstantVectorVal (that inherits ConstantDataSequentials) should cover the rest by handling each Constant element.
| return stable_hash_combine(MO.getType(), MO.getTargetFlags(), | ||
| stable_hash_name(Name), MO.getOffset()); | ||
| stable_hash GVHash = 0; | ||
| if (auto *GVar = dyn_cast<GlobalVariable>(GV)) |
There was a problem hiding this comment.
Seems like we can generate meaningful hash for globals regardless of the presence of name. Should we move the hasName check after this?
There was a problem hiding this comment.
Moved hasName check below.
|
@mingmingl-llvm @nocchijiang @ellishg I think I've addressed all the comments. Could you please review it again?" |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/171/builds/11547 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/16/builds/10011 Here is the relevant piece of the build log for the reference |
kyulee-com
added a commit
that referenced
this pull request
Dec 4, 2024
This reverts commit 1afb81d.
kyulee-com
added a commit
that referenced
this pull request
Dec 4, 2024
This reverts commit 6a0d6fc.
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 update enhances the implementation of structural hashing for global variables, using their initial contents. Private global variables or constants are often used for metadata, where their names are not unique. This can lead to the creation of different hash results although they could be merged by the linker as they are effectively identical.
This change significantly improves size reduction by an additional 1% on the LLD binary when the global function outliner and merger are enabled together. As discussed in the RFC https://discourse.llvm.org/t/loh-conflicting-with-machineoutliner/83279/8?u=kyulee-com, if we disable or relocate the LOH pass, the size impact could increase to 4%.