This is an archive of the discontinued LLVM Phabricator instance.

[PGO] Setting ValueProfNode Array's Alignment
ClosedPublic

Authored by qiongsiwu1 on Feb 17 2023, 2:20 PM.

Details

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

Diff Detail

Event Timeline

qiongsiwu1 created this revision.Feb 17 2023, 2:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 2:20 PM
qiongsiwu1 requested review of this revision.Feb 17 2023, 2:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 2:20 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
MaskRay added inline comments.
compiler-rt/lib/profile/InstrProfiling.h
30 ↗(On Diff #498504)

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.

clang-format and address code review.

qiongsiwu1 marked an inline comment as done.Feb 21 2023, 7:09 AM
w2yehia accepted this revision.Feb 21 2023, 7:45 PM

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.

This revision is now accepted and ready to land.Feb 21 2023, 7:45 PM
This revision now requires review to proceed.Feb 21 2023, 9:53 PM
MaskRay requested changes to this revision.Feb 21 2023, 10:05 PM

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.

This revision now requires changes to proceed.Feb 21 2023, 10:05 PM
qiongsiwu1 added a comment.EditedFeb 22 2023, 10:05 AM

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?

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.

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 marked an inline comment as done.Feb 23 2023, 9:26 AM
MaskRay accepted this revision.Feb 23 2023, 11:26 AM
MaskRay added inline comments.
llvm/test/Instrumentation/InstrProfiling/align32.ll
6

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.

This revision is now accepted and ready to land.Feb 23 2023, 11:26 AM

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.

qiongsiwu1 marked an inline comment as done.Feb 23 2023, 12:18 PM
qiongsiwu1 edited the summary of this revision. (Show Details)Feb 23 2023, 12:25 PM
This revision was automatically updated to reflect the committed changes.