[PATCH] D144302: [PGO] Setting ValueProfNode Array's Alignment

Thu Feb 23 09:20:49 PST 2023

qiongsiwu1 updated this revision to Diff 499891.
qiongsiwu1 added a comment.

In D144302#4145194 <https://reviews.llvm.org/D144302#4145194>, @MaskRay wrote:

> On a Linux x86-64 system, a `-m32` compile shows that `sizeof(ValueProfNode) == 20`. The alignment of `uint64_t` is 4, so `(uint64_t, uint64_t, struct ValueProfNode *)` as there is no alignment padding.

Thanks! This patch is updated to use the ABI alignment instead of a hardcoded one. A new test is added for the ILP32 case since we need to set the datalayout for the test to trigger the 4 byte alignment. If there are ways we can reuse the same existing test, please let me know and I will consolidate the tests.

On another note, `INSTR_PROF_DATA_ALIGNMENT` has the same problem. Its alignment should be 4 when compiled with `clang -m32 -Xclang -triple=i386-linux-gnu_ilp32 -fprofile-generate`. Its size is 36. The instprof pass hardcodes the alignment to 8.

I think it is less of a concern because the NumData calculation does not rely on subtracting pointers to array elements. It does the subtraction and scaling using `intptr_t` (https://github.com/llvm/llvm-project/blob/981e3a35a14541afc6fa338abf3f31895b80eed9/compiler-rt/lib/profile/InstrProfilingBuffer.c#L54). If there is a need to fix `INSTR_PROF_DATA_ALIGNMENT`, I can do it with a different patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144302/new/

https://reviews.llvm.org/D144302

Files:
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
  llvm/test/Instrumentation/InstrProfiling/align32.ll
  llvm/test/Instrumentation/InstrProfiling/icall-comdat.ll


Index: llvm/test/Instrumentation/InstrProfiling/icall-comdat.ll
===================================================================

--- llvm/test/Instrumentation/InstrProfiling/icall-comdat.ll
+++ llvm/test/Instrumentation/InstrProfiling/icall-comdat.ll
@@ -10,7 +10,11 @@
 ; RUN: opt < %s -mtriple=mips64-unknown-linux -passes=instrprof -vp-static-alloc=true -S | FileCheck %s --check-prefix=STATIC-SEXT
 ; RUN: opt < %s -mtriple=x86_64-unknown-linux -passes=instrprof -vp-static-alloc=false -S | FileCheck %s --check-prefix=DYN
 
-
+;; Check that counters have the correct alignments.
+; RUN: opt %s -mtriple=powerpc64-unknown-linux -passes=instrprof -S | FileCheck %s --check-prefix=ALIGN
+; RUN: opt %s -mtriple=powerpc-ibm-aix -passes=instrprof -S | FileCheck %s --check-prefix=ALIGN
+; 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
 
 @__profn_foo = private constant [3 x i8] c"foo"
 @__profn_bar = private constant [3 x i8] c"bar"
@@ -62,3 +66,12 @@
 ; STATIC: declare void @__llvm_profile_instrument_target(i64, ptr, i32)
 ; STATIC-EXT: declare void @__llvm_profile_instrument_target(i64, ptr, i32 zeroext)
 ; STATIC-SEXT: declare void @__llvm_profile_instrument_target(i64, ptr, i32 signext)
+
+; ALIGN: @__profc_foo = private global {{.*}} section "__llvm_prf_cnts",{{.*}} align 8
+; ALIGN: @__profvp_foo = private global {{.*}} section "__llvm_prf_vals",{{.*}} align 8
+; ALIGN: @__profd_foo = private global {{.*}} section "__llvm_prf_data",{{.*}} align 8
+; 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
Index: llvm/test/Instrumentation/InstrProfiling/align32.ll
===================================================================
--- /dev/null
+++ llvm/test/Instrumentation/InstrProfiling/align32.ll
@@ -0,0 +1,37 @@
+;; Check that counters have the correct alignments.
+; RUN: opt %s -passes=instrprof -S | FileCheck %s --check-prefix=ALIGN32
+
+target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128"
+target triple = "i386-unknown-linux-gnu_ilp32"
+
+ at __profn_foo = private constant [3 x i8] c"foo"
+ at __profn_bar = private constant [3 x i8] c"bar"
+
+define i32 @foo(ptr ) {
+  call void @llvm.instrprof.increment(ptr @__profn_foo, i64 12884901887, i32 1, i32 0)
+  %2 = ptrtoint ptr %0 to i64
+  call void @llvm.instrprof.value.profile(ptr @__profn_foo, i64 12884901887, i64 %2, i32 0, i32 0)
+  %3 = tail call i32 %0()
+  ret i32 %3
+}
+
+$bar = comdat any
+
+define i32 @bar(ptr ) comdat {
+entry:
+  call void @llvm.instrprof.increment(ptr @__profn_bar, i64 12884901887, i32 1, i32 0)
+  %1 = ptrtoint ptr %0 to i64
+  call void @llvm.instrprof.value.profile(ptr @__profn_bar, i64 12884901887, i64 %1, i32 0, i32 0)
+  %2 = tail call i32 %0()
+  ret i32 %2
+}
+
+; Function Attrs: nounwind
+declare void @llvm.instrprof.increment(ptr, i64, i32, i32) #0
+
+; Function Attrs: nounwind
+declare void @llvm.instrprof.value.profile(ptr, i64, i64, i32, i32) #0
+
+attributes #0 = { nounwind }
+
+; ALIGN32: @__llvm_prf_vnodes = private global {{.*}} section "__llvm_prf_vnds", align 4
Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===================================================================
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -1180,6 +1180,7 @@
       Constant::getNullValue(VNodesTy), getInstrProfVNodesVarName());
   VNodesVar->setSection(
       getInstrProfSectionName(IPSK_vnodes, TT.getObjectFormat()));
+  VNodesVar->setAlignment(M->getDataLayout().getABITypeAlign(VNodesTy));
   // VNodesVar is used by runtime but not referenced via relocation by other
   // sections. Conservatively make it linker retained.
   UsedVars.push_back(VNodesVar);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D144302.499891.patch
Type: text/x-patch
Size: 4265 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230223/d97fe370/attachment.bin>
