[llvm] fd0ca2c - Revert "Reland "[pgo] Avoid introducing relocations by using private alias""
Hans Wennborg via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 21 06:29:19 PST 2022
Author: Hans Wennborg
Date: 2022-12-21T15:29:00+01:00
New Revision: fd0ca2c4e935cb2d8de95f016d7a08229af695da
URL: https://github.com/llvm/llvm-project/commit/fd0ca2c4e935cb2d8de95f016d7a08229af695da
DIFF: https://github.com/llvm/llvm-project/commit/fd0ca2c4e935cb2d8de95f016d7a08229af695da.diff
LOG: Revert "Reland "[pgo] Avoid introducing relocations by using private alias""
This caused lld on mac to assert when building instrumented clang (or
instrumented code in general). See comment on the code review for
reproducer.
> In many cases, we can use an alias to avoid a symbolic relocations,
> instead of using the public, interposable symbol. When the instrumented
> function is in a COMDAT, we can use a hidden alias, and still avoid
> references to discarded sections.
>
> New compiler-rt tests are Linux only for now.
>
> Previous versions of this patch allowed the compiler to name the
> generated alias, but that would only be valid when the functions were
> local. Since the alias may be used across TUs we use a more
> deterministic naming convention, and add a `.local` suffix to the alias
> name just as we do for relative vtables aliases.
>
> Reviewed By: phosek
>
> Differential Revision: https://reviews.llvm.org/D137982
This reverts commit c42e50fede53bbcce79095e7c8115f26826c81ae.
Added:
Modified:
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
llvm/test/Transforms/PGOProfile/comdat.ll
Removed:
compiler-rt/test/profile/Linux/instrprof-discarded-comdat.cpp
llvm/test/Transforms/PGOProfile/prof_avoid_relocs.ll
################################################################################
diff --git a/compiler-rt/test/profile/Linux/instrprof-discarded-comdat.cpp b/compiler-rt/test/profile/Linux/instrprof-discarded-comdat.cpp
deleted file mode 100644
index da2707a6856a4..0000000000000
--- a/compiler-rt/test/profile/Linux/instrprof-discarded-comdat.cpp
+++ /dev/null
@@ -1,50 +0,0 @@
-// Check that instrprof does not introduce references to discarded sections when
-// using comdats.
-//
-// Occasionally, it is possible that the same function can be compiled in
-//
diff erent TUs with slightly
diff erent linkages, e.g., due to
diff erent
-// compiler options. However, if these are comdat functions, a single
-// implementation will be chosen at link time. we want to ensure that the
-// profiling data does not contain a reference to the discarded section.
-
-// REQUIRES: linux
-// RUN: mkdir -p %t.d
-// RUN: %clangxx_pgogen -O2 -fPIC -ffunction-sections -fdata-sections -c %s -o %t.d/a1.o -DOBJECT_1 -mllvm -disable-preinline
-// RUN: %clangxx_pgogen -O2 -fPIC -ffunction-sections -fdata-sections -c %s -o %t.d/a2.o
-// RUN: %clangxx -fPIC -shared -o %t.d/liba.so %t.d/a1.o %t.d/a2.o 2>&1 | FileCheck %s --allow-empty
-
-// Ensure that we don't get an error when linking
-// CHECK-NOT: relocation refers to a discarded section: .text._ZN1CIiE1fEi
-
-template <typename T> struct C {
- void f(T x);
- int g(T x) {
- f(x);
- return v;
- }
- int v;
-};
-
-template <typename T>
-#ifdef OBJECT_1
-__attribute__((weak))
-#else
-__attribute__((noinline))
-#endif
-void C<T>::f(T x) {
- v += x;
-}
-
-#ifdef OBJECT_1
-int foo() {
- C<int> c;
- c.f(1);
- return c.g(2);
-}
-#else
-int bar() {
- C<int> c;
- c.f(3);
- return c.g(4);
-}
-#endif
diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
index 8f5e8172bf6ca..3af3f908ea5b2 100644
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -823,36 +823,6 @@ static inline bool shouldRecordFunctionAddr(Function *F) {
return F->hasAddressTaken() || F->hasLinkOnceLinkage();
}
-static inline Constant *getFuncAddrForProfData(Function *Fn) {
- auto *Int8PtrTy = Type::getInt8PtrTy(Fn->getContext());
- // Store a nullptr in __llvm_profd, if we shouldn't use a real address
- if (!shouldRecordFunctionAddr(Fn))
- return ConstantPointerNull::get(Int8PtrTy);
-
- // If we can't use an alias, we must use the public symbol, even though this
- // may require a symbolic relocation. When the function has local linkage, we
- // can use the symbol directly without introducing relocations.
- if (Fn->isDeclarationForLinker() || Fn->hasLocalLinkage())
- return ConstantExpr::getBitCast(Fn, Int8PtrTy);
-
- // When possible use a private alias to avoid symbolic relocations.
- auto *GA = GlobalAlias::create(GlobalValue::LinkageTypes::PrivateLinkage,
- Fn->getName() + ".local", Fn);
-
- // When the instrumented function is a COMDAT function, we cannot use a
- // private alias. If we did, we would create reference to a local label in
- // this function's section. If this version of the function isn't selected by
- // the linker, then the metadata would introduce a reference to a discarded
- // section. So, for COMDAT functions, we need to adjust the linkage of the
- // alias. Using hidden visibility avoids a dynamic relocation and an entry in
- // the dynamic symbol table.
- if (Fn->hasComdat()) {
- GA->setLinkage(Fn->getLinkage());
- GA->setVisibility(GlobalValue::VisibilityTypes::HiddenVisibility);
- }
- return ConstantExpr::getBitCast(GA, Int8PtrTy);
-}
-
static bool needsRuntimeRegistrationOfSectionRange(const Triple &TT) {
// Don't do this for Darwin. compiler-rt uses linker magic.
if (TT.isOSDarwin())
@@ -1044,7 +1014,9 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfInstBase *Inc) {
};
auto *DataTy = StructType::get(Ctx, makeArrayRef(DataTypes));
- Constant *FunctionAddr = getFuncAddrForProfData(Fn);
+ Constant *FunctionAddr = shouldRecordFunctionAddr(Fn)
+ ? ConstantExpr::getBitCast(Fn, Int8PtrTy)
+ : ConstantPointerNull::get(Int8PtrTy);
Constant *Int16ArrayVals[IPVK_Last + 1];
for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
diff --git a/llvm/test/Transforms/PGOProfile/comdat.ll b/llvm/test/Transforms/PGOProfile/comdat.ll
index 765a77538a9b1..9f5c0ee848ca5 100644
--- a/llvm/test/Transforms/PGOProfile/comdat.ll
+++ b/llvm/test/Transforms/PGOProfile/comdat.ll
@@ -4,8 +4,6 @@
$linkonceodr = comdat any
$weakodr = comdat any
-$weak = comdat any
-$linkonce = comdat any
;; profc/profd have hash suffixes. This definition doesn't have value profiling,
;; so definitions with the same name in other modules must have the same CFG and
@@ -29,32 +27,3 @@ define linkonce_odr void @linkonceodr() comdat {
define weak_odr void @weakodr() comdat {
ret void
}
-
-;; weak in a comdat is not renamed. There is no guarantee that definitions in
-;; other modules don't have value profiling. profd should be conservatively
-;; non-private to prevent a caller from referencing a non-prevailing profd,
-;; causing a linker error.
-; ELF: @__profc_weak = weak hidden global {{.*}} comdat, align 8
-; ELF: @__profd_weak = weak hidden global {{.*}} comdat($__profc_weak), align 8
-; COFF: @__profc_weak = weak hidden global {{.*}} comdat, align 8
-; COFF: @__profd_weak = weak hidden global {{.*}} comdat, align 8
-define weak void @weak() comdat {
- ret void
-}
-
-;; profc/profd have hash suffixes. This definition doesn't have value profiling,
-;; so definitions with the same name in other modules must have the same CFG and
-;; cannot have value profiling, either. profd can be made private for ELF.
-; ELF: @__profc_linkonce.[[#]] = linkonce hidden global {{.*}} comdat, align 8
-; ELF: @__profd_linkonce.[[#]] = private global {{.*}} comdat($__profc_linkonce.[[#]]), align 8
-; COFF: @__profc_linkonce.[[#]] = linkonce hidden global {{.*}} comdat, align 8
-; COFF: @__profd_linkonce.[[#]] = linkonce hidden global {{.*}} comdat, align 8
-define linkonce void @linkonce() comdat {
- ret void
-}
-
-; Check that comdat aliases are hidden for all linkage types
-; ELF: @linkonceodr.local = linkonce_odr hidden alias void (), ptr @linkonceodr
-; ELF: @weakodr.local = weak_odr hidden alias void (), ptr @weakodr
-; ELF: @weak.local = weak hidden alias void (), ptr @weak
-; ELF: @linkonce.local = linkonce hidden alias void (), ptr @linkonce
diff --git a/llvm/test/Transforms/PGOProfile/prof_avoid_relocs.ll b/llvm/test/Transforms/PGOProfile/prof_avoid_relocs.ll
deleted file mode 100644
index ee32a303a4306..0000000000000
--- a/llvm/test/Transforms/PGOProfile/prof_avoid_relocs.ll
+++ /dev/null
@@ -1,87 +0,0 @@
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals
-; RUN: opt -S -passes=pgo-instr-gen,instrprof < %s | FileCheck %s
-
-target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
-target triple = "x86_64-unknown-linux-gnu"
-
-;; Test that we use private aliases to reference function addresses inside profile data
-; CHECK: @__profd_foo = private global {{.*}}, ptr @foo.local,
-; CHECK-NOT: @__profd_foo = private global {{.*}}, ptr @foo,
-
-; CHECK: @[[__PROFC_WEAK:[a-zA-Z0-9_$"\\.-]+]] = weak hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
-; CHECK: @[[__PROFD_WEAK:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 -5028622335731970946, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_weak to i64), i64 ptrtoint (ptr @__profd_weak to i64)), ptr @weak.local, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_weak), align 8
-; CHECK: @[[__PROFC_LINKONCE:[a-zA-Z0-9_$"\\.-]+]] = linkonce hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
-; CHECK: @[[__PROFD_LINKONCE:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 -121947654961992603, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_linkonce to i64), i64 ptrtoint (ptr @__profd_linkonce to i64)), ptr @linkonce.local, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_linkonce), align 8
-; CHECK: @[[__PROFC_WEAKODR:[a-zA-Z0-9_$"\\.-]+]] = weak_odr hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
-; CHECK: @[[__PROFD_WEAKODR:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 -4807837289933096997, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_weakodr to i64), i64 ptrtoint (ptr @__profd_weakodr to i64)), ptr @weakodr.local, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_weakodr), align 8
-; CHECK: @[[__PROFC_LINKONCEODR:[a-zA-Z0-9_$"\\.-]+]] = linkonce_odr hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
-; CHECK: @[[__PROFD_LINKONCEODR:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 4214081367395809689, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_linkonceodr to i64), i64 ptrtoint (ptr @__profd_linkonceodr to i64)), ptr @linkonceodr.local, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_linkonceodr), align 8
-; CHECK: @[[__PROFC_AVAILABLE_EXTERNALLY_742261418966908927:[a-zA-Z0-9_$"\\.-]+]] = linkonce_odr hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat, align 8
-; CHECK: @[[__PROFD_AVAILABLE_EXTERNALLY_742261418966908927:[a-zA-Z0-9_$"\\.-]+]] = private global { i64, i64, i64, ptr, ptr, i32, [2 x i16] } { i64 -8510055422695886042, i64 742261418966908927, i64 sub (i64 ptrtoint (ptr @__profc_available_externally.742261418966908927 to i64), i64 ptrtoint (ptr @__profd_available_externally.742261418966908927 to i64)), ptr null, ptr null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profc_available_externally.742261418966908927), align 8
-
-;; Ensure when not instrumenting a non-comdat function, then if we generate an
-;; alias, then it is private. We check comdat versions in comdat.ll
-; CHECK: @foo.local = private alias i32 (i32), ptr @foo
-; CHECK: @[[WEAK_2:[a-zA-Z0-9_$"\\.-]+]].local = private alias void (), ptr @weak
-; CHECK: @[[LINKONCE_3:[a-zA-Z0-9_$"\\.-]+]].local = private alias void (), ptr @linkonce
-; CHECK: @[[WEAKODR_4:[a-zA-Z0-9_$"\\.-]+]].local = private alias void (), ptr @weakodr
-; CHECK: @[[LINKONCEODR_5:[a-zA-Z0-9_$"\\.-]+]].local = private alias void (), ptr @linkonceodr
-
-;; We should never generate an alias for available_externally functions
-; CHECK-NOT: @[[AVAILABLE_EXTERNALLY_6:[a-zA-Z0-9_$"\\.-]+]] = private alias void (), ptr @available_externally
-
-define i32 @foo(i32 %0) {
-; CHECK-LABEL: @foo(
-; CHECK-NEXT: entry:
-; CHECK-NEXT: [[PGOCOUNT:%.*]] = load i64, ptr @__profc_foo, align 8
-; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
-; CHECK-NEXT: store i64 [[TMP1]], ptr @__profc_foo, align 8
-; CHECK-NEXT: ret i32 0
-entry:
- ret i32 0
-}
-
-define weak void @weak() {
-; CHECK-LABEL: @weak(
-; CHECK-NEXT: [[PGOCOUNT:%.*]] = load i64, ptr @__profc_weak, align 8
-; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
-; CHECK-NEXT: store i64 [[TMP1]], ptr @__profc_weak, align 8
-; CHECK-NEXT: ret void
- ret void
-}
-
-define linkonce void @linkonce() {
-; CHECK-LABEL: @linkonce(
-; CHECK-NEXT: [[PGOCOUNT:%.*]] = load i64, ptr @__profc_linkonce, align 8
-; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
-; CHECK-NEXT: store i64 [[TMP1]], ptr @__profc_linkonce, align 8
-; CHECK-NEXT: ret void
- ret void
-}
-
-define weak_odr void @weakodr() {
-; CHECK-LABEL: @weakodr(
-; CHECK-NEXT: [[PGOCOUNT:%.*]] = load i64, ptr @__profc_weakodr, align 8
-; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
-; CHECK-NEXT: store i64 [[TMP1]], ptr @__profc_weakodr, align 8
-; CHECK-NEXT: ret void
- ret void
-}
-
-define linkonce_odr void @linkonceodr() {
-; CHECK-LABEL: @linkonceodr(
-; CHECK-NEXT: [[PGOCOUNT:%.*]] = load i64, ptr @__profc_linkonceodr, align 8
-; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
-; CHECK-NEXT: store i64 [[TMP1]], ptr @__profc_linkonceodr, align 8
-; CHECK-NEXT: ret void
- ret void
-}
-
-define available_externally void @available_externally(){
-; CHECK-LABEL: @available_externally(
-; CHECK-NEXT: [[PGOCOUNT:%.*]] = load i64, ptr @__profc_available_externally.742261418966908927, align 8
-; CHECK-NEXT: [[TMP1:%.*]] = add i64 [[PGOCOUNT]], 1
-; CHECK-NEXT: store i64 [[TMP1]], ptr @__profc_available_externally.742261418966908927, align 8
-; CHECK-NEXT: ret void
- ret void
-}
More information about the llvm-commits
mailing list