[PGO] Setting ValueProfNode Array's Alignment

[qiongsiwu1]

Summary

instrprof currently does not set __llvm_prf_vnds's alignment after creating it. The consequence is that the alignment is set to 16 later (https://github.com/llvm/llvm-project/blob/c0f3ac1d0015fd051144a987ff500b888a32be86/llvm/lib/IR/DataLayout.cpp#L1019). This can lead to undefined behaviour when we calculate NumVNodes in lprofGetLoadModuleSignature (https://github.com/llvm/llvm-project/blob/c0f3ac1d0015fd051144a987ff500b888a32be86/compiler-rt/lib/profile/InstrProfilingMerge.c#L32). The reason is that when the __llvm_prf_vnds array is 16 byte aligned, __llvm_profile_end_vnodes() - __llvm_profile_begin_vnodes() may not be a multiple of the size of ValueProfNode (which is 24, 20 on 32 bit targets).

This patch sets __llvm_prf_vnds's alignment to its ABI alignment, which always divides its size. Then __llvm_profile_end_vnodes() - __llvm_profile_begin_vnodes() will be a multiple of sizeof(ValueProfNode).

[MaskRay]

compiler-rt/lib/profile/InstrProfiling.h

30

drop ending period. The format typically matches clang diagnostics which use no capitalization or trailing period.

llvm/test/Instrumentation/InstrProfiling/alignment.ll

1 ↗

(On Diff #498504)

It's not necessary to add a new test. Just find an existing one and more CHECK lines there.

[qiongsiwu1]

clang-format and address code review.

[w2yehia]

LGTM

llvm/test/Instrumentation/InstrProfiling/icall-comdat.ll

21

to be consistent with the existing position, the expected lines should be moved to the bottom of the file.

[MaskRay]

On ILP32 targets, sizeof(ValueProfNode) = 20. The forced alignment 8 is not a divisor of 20 and there may be alignment padding between two ValueProfNode instances.

[qiongsiwu1]

In D144302#4143493, @MaskRay wrote:

On ILP32 targets, sizeof(ValueProfNode) = 20. The forced alignment 8 is not a divisor of 20 and there may be alignment padding between two ValueProfNode instances.

Ah thanks so much for catching this!

I did some experiments but I was not able to reproduce sizeof(ValueProfNode) = 20 with ILP32 or with 32 bit pointers. ValueProfNode's definition (https://github.com/llvm/llvm-project/blob/9248b5d3fca19a31fde76e7df294d75f52779641/compiler-rt/include/profile/InstrProfData.inc#L95) states that it has 3 fields {int64, int64, ptr}. There are no fields of type long. When compiling with 32 bit pointers, the size of the structure is 24, instead of 20 due to padding (at least during my experiments on AIX).

An additional experiment I tried was to use getABITypeAlign(VNodesTy) and getPrefTypeAlign(VNodesTy) to compute the alignment. Both methods returned 8 when I ran opt with command opt -mtriple=i386-unknown-linux -target-abi ilp32 -passes=instrprof -S. Maybe I am not doing my experiments correctly? Is there a specific target triple I should try?

[MaskRay]

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.

[qiongsiwu1]

In 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.

[qiongsiwu1]

[MaskRay]

llvm/test/Instrumentation/InstrProfiling/align32.ll

5 ↗

(On Diff #499891)

IIUI gnu_ilp32 is only for normally-64-bit architectures.

Use i386-unknown-linux-gnu. If the other test supports -mtriple=i386-unknown-linux-gnu, don't add this new test file.

[qiongsiwu1]

Address code review - fixing target triple used in the new test. The new test is added because we need a specific data layout to test out ILP32. Are there ways to specify a datalayout for a specific run (similar to how we specify target triples)? If so I will consolidate the tests.

compiler-rt/include/profile/InstrProfData.inc

#define INSTR_PROF_VALUE_PROF_FUNC_STR \
         INSTR_PROF_QUOTE(INSTR_PROF_VALUE_PROF_FUNC)
 #define INSTR_PROF_VALUE_PROF_MEMOP_FUNC __llvm_profile_instrument_memop
 #define INSTR_PROF_VALUE_PROF_MEMOP_FUNC_STR                                   \
   INSTR_PROF_QUOTE(INSTR_PROF_VALUE_PROF_MEMOP_FUNC)
 
 /* InstrProfile per-function control data alignment.  */
 #define INSTR_PROF_DATA_ALIGNMENT 8
 
+/* InstrProfile VNodes alilgnment. */
+#define INSTR_PROF_VNODE_ALIGNMENT 8
+
 /* The data structure that represents a tracked value by the
  * value profiler.
  */
 typedef struct InstrProfValueData {
   /* Profiled value. */
   uint64_t Value;
   /* Number of times the value appears in the training run. */
   uint64_t Count;

compiler-rt/lib/profile/InstrProfiling.h

 /*===- InstrProfiling.h- Support library for PGO instrumentation ----------===*\
 |*
 |* 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
 |*
 \*===----------------------------------------------------------------------===*/
 
 #ifndef PROFILE_INSTRPROFILING_H_
 #define PROFILE_INSTRPROFILING_H_
 
 #include "InstrProfilingPort.h"
+#include <assert.h>
 #include <stdio.h>
 
 #define INSTR_PROF_VISIBILITY COMPILER_RT_VISIBILITY
 #include "profile/InstrProfData.inc"
 
 enum ValueKind {
 #define VALUE_PROF_KIND(Enumerator, Value, Descr) Enumerator = Value,
 #include "profile/InstrProfData.inc"
 };
 
 typedef void *IntPtrT;
 typedef struct COMPILER_RT_ALIGNAS(INSTR_PROF_DATA_ALIGNMENT)
     __llvm_profile_data {
 #define INSTR_PROF_DATA(Type, LLVMType, Name, Initializer) Type Name;
 #include "profile/InstrProfData.inc"
 } __llvm_profile_data;
+static_assert(INSTR_PROF_DATA_ALIGNMENT >= _Alignof(__llvm_profile_data),
+              "INSTR_PROF_DATA_ALIGNMENT is too small");
 
 typedef struct __llvm_profile_header {
 #define INSTR_PROF_RAW_HEADER(Type, Name, Initializer) Type Name;
 #include "profile/InstrProfData.inc"
 } __llvm_profile_header;
 
-typedef struct ValueProfNode * PtrToNodeT;
-typedef struct ValueProfNode {
+typedef struct ValueProfNode *PtrToNodeT;
+typedef struct COMPILER_RT_ALIGNAS(INSTR_PROF_VNODE_ALIGNMENT) ValueProfNode {
 #define INSTR_PROF_VALUE_NODE(Type, LLVMType, Name, Initializer) Type Name;
 #include "profile/InstrProfData.inc"
 } ValueProfNode;
+static_assert(INSTR_PROF_VNODE_ALIGNMENT >= _Alignof(ValueProfNode),
+              "INSTR_PROF_VNODE_ALIGNMENT is too small");
 
 /*!
  * \brief Return 1 if profile counters are continuously synced to the raw
  * profile via an mmap(). This is in contrast to the default mode, in which
  * the raw profile is written out at program exit time.
  */
 int __llvm_profile_is_continuous_mode_enabled(void);
 

 
 /*! \brief Check if profile in buffer matches the current binary.
  *
  *  Returns 0 (success) if the profile data in buffer \p Profile with size
  *  \p Size was generated by the same binary and therefore matches
  *  structurally the in-process counters. If the profile data in buffer is
  *  not compatible, the interface returns 1 (failure).
  */
-int __llvm_profile_check_compatibility(const char *Profile,
-                                       uint64_t Size);
+int __llvm_profile_check_compatibility(const char *Profile, uint64_t Size);
 
 /*!
  * \brief Counts the number of times a target value is seen.
  *
  * Records the target value for the CounterIndex if not seen before. Otherwise,
  * increments the counter associated w/ the target value.
  * void __llvm_profile_instrument_target(uint64_t TargetValue, void *Data,
  *                                       uint32_t CounterIndex);
  */
 void INSTR_PROF_VALUE_PROF_FUNC(
 #define VALUE_PROF_FUNC_PARAM(ArgType, ArgName, ArgLLVMType) ArgType ArgName
 #include "profile/InstrProfData.inc"
-    );
 
 void __llvm_profile_instrument_target_value(uint64_t TargetValue, void *Data,
                                             uint32_t CounterIndex,
                                             uint64_t CounterValue);
 
 /*!
  * \brief Write instrumentation data to the current file.
  *

llvm/include/llvm/ProfileData/InstrProfData.inc

#define INSTR_PROF_VALUE_PROF_FUNC_STR \
         INSTR_PROF_QUOTE(INSTR_PROF_VALUE_PROF_FUNC)
 #define INSTR_PROF_VALUE_PROF_MEMOP_FUNC __llvm_profile_instrument_memop
 #define INSTR_PROF_VALUE_PROF_MEMOP_FUNC_STR                                   \
   INSTR_PROF_QUOTE(INSTR_PROF_VALUE_PROF_MEMOP_FUNC)
 
 /* InstrProfile per-function control data alignment.  */
 #define INSTR_PROF_DATA_ALIGNMENT 8
 
+/* InstrProfile VNodes alilgnment. */
+#define INSTR_PROF_VNODE_ALIGNMENT 8
+
 /* The data structure that represents a tracked value by the
  * value profiler.
  */
 typedef struct InstrProfValueData {
   /* Profiled value. */
   uint64_t Value;
   /* Number of times the value appears in the training run. */
   uint64_t Count;

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

#include "llvm/ProfileData/InstrProfData.inc"
   auto *VNodeTy = StructType::get(Ctx, ArrayRef(VNodeTypes));
 
   ArrayType *VNodesTy = ArrayType::get(VNodeTy, NumCounters);
   auto *VNodesVar = new GlobalVariable(
       *M, VNodesTy, false, GlobalValue::PrivateLinkage,
       Constant::getNullValue(VNodesTy), getInstrProfVNodesVarName());
   VNodesVar->setSection(
       getInstrProfSectionName(IPSK_vnodes, TT.getObjectFormat()));
+  VNodesVar->setAlignment(Align(INSTR_PROF_VNODE_ALIGNMENT));
   // VNodesVar is used by runtime but not referenced via relocation by other
   // sections. Conservatively make it linker retained.
   UsedVars.push_back(VNodesVar);
 }
 
 void InstrProfiling::emitNameData() {
   std::string UncompressedData;
 

llvm/test/Instrumentation/InstrProfiling/icall-comdat.ll

 ;; 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
 ; RUN: opt < %s -mtriple=s390x-unknown-linux -passes=instrprof -vp-static-alloc=true -S | FileCheck %s --check-prefix=STATIC-EXT
 ; RUN: opt < %s -mtriple=powerpc64-unknown-linux -passes=instrprof -vp-static-alloc=true -S | FileCheck %s --check-prefix=STATIC-EXT
 ; RUN: opt < %s -mtriple=sparc64-unknown-linux -passes=instrprof -vp-static-alloc=true -S | FileCheck %s --check-prefix=STATIC-EXT
 ; RUN: opt < %s -mtriple=mips-unknown-linux -passes=instrprof -vp-static-alloc=true -S | FileCheck %s --check-prefix=STATIC-SEXT
 ; 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"
+; 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
 
 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
 }