[InstrProf][X86] Mark non-directly accessed globals as large #74778
Conversation
This file contains 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
We'd like to make various instrprof globals large to make them not contribute to relocation pressure since there are no direct accesses to them in the module. Similar to what was done for asan_globals in llvm#74514. This affects the __llvm_prf_vals, __llvm_prf_vnds, and __llvm_prf_names sections.
llvmbot
added
PGO
|
@llvm/pr-subscribers-pgo @llvm/pr-subscribers-llvm-transforms Author: Arthur Eubanks (aeubanks) ChangesWe'd like to make various instrprof globals large to make them not Similar to what was done for asan_globals in #74514. This affects the __llvm_prf_vals, __llvm_prf_vnds, and __llvm_prf_names Full diff: https://github.com/llvm/llvm-project/pull/74778.diff 5 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Instrumentation.h b/llvm/include/llvm/Transforms/Instrumentation.h
index 3035cdeeacf68..0e97b7d8de3ad 100644
--- a/llvm/include/llvm/Transforms/Instrumentation.h
+++ b/llvm/include/llvm/Transforms/Instrumentation.h
@@ -49,6 +49,11 @@ GlobalVariable *createPrivateGlobalForString(Module &M, StringRef Str,
// Returns nullptr on failure.
Comdat *getOrCreateFunctionComdat(Function &F, Triple &T);
+// Place global in a large section for x86-64 ELF binaries to mitigate
+// relocation pressure. Should typically be used for metadata globals that
+// aren't directly accessed in the module.
+void setGlobalVariableLargeSection(Triple &TargetTriple, GlobalVariable &GV);
+
// Insert GCOV profiling instrumentation
struct GCOVOptions {
static GCOVOptions getDefault();
diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index 3a3b41f5dc87d..b175e6f93f3e8 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -2146,9 +2146,7 @@ ModuleAddressSanitizer::CreateMetadataGlobal(Module &M, Constant *Initializer,
Metadata->setSection(getGlobalMetadataSection());
// Place metadata in a large section for x86-64 ELF binaries to mitigate
// relocation pressure.
- if (TargetTriple.getArch() == Triple::x86_64 &&
- TargetTriple.getObjectFormat() == Triple::ELF)
- Metadata->setCodeModel(CodeModel::Large);
+ setGlobalVariableLargeSection(TargetTriple, *Metadata);
return Metadata;
}
diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
index 10258e2546791..d3282779d9f5f 100644
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -49,6 +49,7 @@
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/TargetParser/Triple.h"
+#include "llvm/Transforms/Instrumentation.h"
#include "llvm/Transforms/Instrumentation/PGOInstrumentation.h"
#include "llvm/Transforms/Utils/BasicBlockUtils.h"
#include "llvm/Transforms/Utils/ModuleUtils.h"
@@ -1294,6 +1295,7 @@ void InstrProfiling::createDataVariable(InstrProfCntrInstBase *Inc) {
*M, ValuesTy, false, Linkage, Constant::getNullValue(ValuesTy),
getVarName(Inc, getInstrProfValuesVarPrefix(), Renamed));
ValuesVar->setVisibility(Visibility);
+ setGlobalVariableLargeSection(TT, *ValuesVar);
ValuesVar->setSection(
getInstrProfSectionName(IPSK_vals, TT.getObjectFormat()));
ValuesVar->setAlignment(Align(8));
@@ -1422,6 +1424,7 @@ void InstrProfiling::emitVNodes() {
auto *VNodesVar = new GlobalVariable(
*M, VNodesTy, false, GlobalValue::PrivateLinkage,
Constant::getNullValue(VNodesTy), getInstrProfVNodesVarName());
+ setGlobalVariableLargeSection(TT, *VNodesVar);
VNodesVar->setSection(
getInstrProfSectionName(IPSK_vnodes, TT.getObjectFormat()));
VNodesVar->setAlignment(M->getDataLayout().getABITypeAlign(VNodesTy));
@@ -1449,6 +1452,7 @@ void InstrProfiling::emitNameData() {
GlobalValue::PrivateLinkage, NamesVal,
getInstrProfNamesVarName());
NamesSize = CompressedNameStr.size();
+ setGlobalVariableLargeSection(TT, *NamesVar);
NamesVar->setSection(
getInstrProfSectionName(IPSK_name, TT.getObjectFormat()));
// On COFF, it's important to reduce the alignment down to 1 to prevent the
diff --git a/llvm/lib/Transforms/Instrumentation/Instrumentation.cpp b/llvm/lib/Transforms/Instrumentation/Instrumentation.cpp
index 806afc8fcdf7c..199afbe966dde 100644
--- a/llvm/lib/Transforms/Instrumentation/Instrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/Instrumentation.cpp
@@ -85,3 +85,10 @@ Comdat *llvm::getOrCreateFunctionComdat(Function &F, Triple &T) {
return C;
}
+void llvm::setGlobalVariableLargeSection(Triple &TargetTriple,
+ GlobalVariable &GV) {
+ if (TargetTriple.getArch() == Triple::x86_64 &&
+ TargetTriple.getObjectFormat() == Triple::ELF) {
+ GV.setCodeModel(CodeModel::Large);
+ }
+}
diff --git a/llvm/test/Instrumentation/InstrProfiling/icall-comdat.ll b/llvm/test/Instrumentation/InstrProfiling/icall-comdat.ll
index c90f37c820895..502cef8a73a0e 100644
--- a/llvm/test/Instrumentation/InstrProfiling/icall-comdat.ll
+++ b/llvm/test/Instrumentation/InstrProfiling/icall-comdat.ll
@@ -1,5 +1,4 @@
;; Check that static counters are allocated for value profiler
-
; RUN: opt < %s -mtriple=x86_64-unknown-linux -passes=instrprof -vp-static-alloc=true -S | FileCheck %s --check-prefix=STATIC
; RUN: opt < %s -mtriple=powerpc-unknown-linux -passes=instrprof -vp-static-alloc=true -S | FileCheck %s --check-prefix=STATIC
; RUN: opt < %s -mtriple=sparc-unknown-linux -passes=instrprof -vp-static-alloc=true -S | FileCheck %s --check-prefix=STATIC
@@ -16,6 +15,10 @@
; RUN: opt %s -mtriple=powerpc64-ibm-aix -passes=instrprof -S | FileCheck %s --check-prefix=ALIGN
; RUN: opt %s -mtriple=x86_64-unknown-linux -passes=instrprof -S | FileCheck %s --check-prefix=ALIGN
+;; Check that globals have the proper code model.
+; RUN: opt %s -mtriple=x86_64-unknown-linux -passes=instrprof -S | FileCheck %s --check-prefix=X8664-CODEMODEL
+; RUN: opt %s -mtriple=powerpc-unknown-linux -passes=instrprof -S | FileCheck %s --check-prefix=PPC-CODEMODEL
+
@__profn_foo = private constant [3 x i8] c"foo"
@__profn_bar = private constant [3 x i8] c"bar"
@@ -46,8 +49,8 @@ declare void @llvm.instrprof.value.profile(ptr, i64, i64, i32, i32) #0
attributes #0 = { nounwind }
-; STATIC: @__profvp_foo = private global [1 x i64] zeroinitializer, section "{{[^"]+}}", comdat($__profc_foo)
-; STATIC: @__profvp_bar = private global [1 x i64] zeroinitializer, section "{{[^"]+}}", comdat($__profc_bar)
+; STATIC: @__profvp_foo = private global [1 x i64] zeroinitializer, section "{{[^"]+}}",{{.*}} comdat($__profc_foo)
+; STATIC: @__profvp_bar = private global [1 x i64] zeroinitializer, section "{{[^"]+}}",{{.*}} comdat($__profc_bar)
; STATIC: @__llvm_prf_vnodes
; DYN-NOT: @__profvp_foo
@@ -73,5 +76,15 @@ attributes #0 = { nounwind }
; ALIGN: @__profc_bar = private global {{.*}} section "__llvm_prf_cnts",{{.*}} align 8
; ALIGN: @__profvp_bar = private global {{.*}} section "__llvm_prf_vals",{{.*}} align 8
; ALIGN: @__profd_bar = private global {{.*}} section "__llvm_prf_data",{{.*}} align 8
-; ALIGN: @__llvm_prf_vnodes = private global {{.*}} section "__llvm_prf_vnds", align 8
-; ALIGN: @__llvm_prf_nm = private constant {{.*}} section "__llvm_prf_names", align 1
+; ALIGN: @__llvm_prf_vnodes = private global {{.*}} section "__llvm_prf_vnds",{{.*}} align 8
+; ALIGN: @__llvm_prf_nm = private constant {{.*}} section "__llvm_prf_names",{{.*}} align 1
+
+; X8664-CODEMODEL-NOT: @__profc_foo = {{.*}}, code_model "large"
+; X8664-CODEMODEL: @__profvp_foo = {{.*}}, code_model "large"
+; X8664-CODEMODEL-NOT: @__profd_foo = {{.*}}, code_model "large"
+; X8664-CODEMODEL-NOT: @__profc_bar = {{.*}}, code_model "large"
+; X8664-CODEMODEL: @__profvp_bar = {{.*}}, code_model "large"
+; X8664-CODEMODEL-NOT: @__profd_bar = {{.*}}, code_model "large"
+; X8664-CODEMODEL: @__llvm_prf_vnodes = {{.*}}, code_model "large"
+; X8664-CODEMODEL: @__llvm_prf_nm = {{.*}}, code_model "large"
+; PPC-CODEMODEL-NOT: code_model "large"
|
MaskRay
approved these changes
Dec 8, 2023
| ; ALIGN: @__llvm_prf_vnodes = private global {{.*}} section "__llvm_prf_vnds",{{.*}} align 8 | ||
| ; ALIGN: @__llvm_prf_nm = private constant {{.*}} section "__llvm_prf_names",{{.*}} align 1 | ||
|
|
||
| ; X8664-CODEMODEL-NOT: @__profc_foo = {{.*}}, code_model "large" |
There was a problem hiding this comment.
; X8664-CODEMODEL: @__profc_foo
; X8664-CODEMODEL-NOT: code_model
ditto below to ensure that the check line finds the variables.
MaskRay
reviewed
Dec 8, 2023
| @@ -49,6 +49,11 @@ GlobalVariable *createPrivateGlobalForString(Module &M, StringRef Str, | |||
| // Returns nullptr on failure. | |||
| Comdat *getOrCreateFunctionComdat(Function &F, Triple &T); | |||
|
|
|||
| // Place global in a large section for x86-64 ELF binaries to mitigate | |||
| // relocation pressure. Should typically be used for metadata globals that | |||
There was a problem hiding this comment.
// Place global in a large section for x86-64 ELF binaries to mitigate
// relocation overflow pressure. This can be be used for metadata globals that aren't directly accessed by code, which has no performance concern.
aeubanks
added a commit
that referenced
this pull request
Dec 8, 2023
…74778)" This reverts commit 5507f70. Breaks bots, e.g. https://lab.llvm.org/buildbot/#/builders/232/builds/16374
aeubanks
added a commit
that referenced
this pull request
Dec 8, 2023
…74778) We'd like to make various instrprof globals large to make them not contribute to relocation pressure since there are no direct accesses to them in the module. Similar to what was done for asan_globals in #74514. This affects the __llvm_prf_vals, __llvm_prf_vnds, and __llvm_prf_names sections. The reland fixes platform.ll.
aeubanks
added a commit
to aeubanks/llvm-project
that referenced
this pull request
Dec 14, 2023
…ge code models In llvm#74514 and llvm#74778 we marked various instrumentation-added sections as large. This causes an extra PT_LOAD segment if using the small code model. Since people using the small code model presumably aren't hitting relocation limits, disable this when using the small code model to avoid the extra segment. This is annoying API-wise because we need to pass TargetMachine around to any pass that wants to do this. Module::getCodeModel(), like anything else that reads Module metadata, is unreliable as a source of truth.
aeubanks
added a commit
that referenced
this pull request
Dec 15, 2023
…ge code models (#75542) In #74514 and #74778 we marked various instrumentation-added sections as large. This causes an extra PT_LOAD segment if using the small code model. Since people using the small code model presumably aren't hitting relocation limits, disable this when using the small code model to avoid the extra segment. This uses Module::getCodeModel() which isn't necessarily reliable since it reads module metadata (which right now only the clang frontend sets), but it would be nice to get to a point where we reliably put this sort of information (e.g. PIC/code model/etc) in the IR. This requires duplicating the existing tests since opt/llc currently don't set these metadata. If we get to a point where they do set the code model metadata based on command line arguments then we can deduplicate these tests.
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.
We'd like to make various instrprof globals large to make them not
contribute to relocation pressure since there are no direct accesses
to them in the module.
Similar to what was done for asan_globals in #74514.
This affects the __llvm_prf_vals, __llvm_prf_vnds, and __llvm_prf_names
sections.