Mention the one-time exception for .profraw compatibility

Not keeping version compatibility for the raw profile format would actually make my life easier.
But I was CCed on the Linux kernel news https://www.phoronix.com/scan.php?page=news_item&px=Clang-PGO-For-Linux-Next so I paid additional care in this patch.

rebase

In D104556#2831726, @rnk wrote:

On COFF, it would be more traditional to make these image-relative 32-bit offsets, so they would come out as .long __profc_foo@IMGREL. If you grep for IMGREL in test/, you can see various exception handling and RTTI tables use that structure.

I seem to recall this was also an issue for swift, which wanted label difference relocations.

The absolute function address (used by llvm-profdata to collect indirect call
targets) can be converted to relative as well, but is not done in this patch.

If we're doing a format change, we should think about this a bit. Previously I was considering removing this field altogether and trying to store this information in a new section, but I don't think it's worth it after D102818.

Add more comments to InstrProfilingMerge.c

In D104564#2831690, @rnk wrote:

I was going to say, I wonder if it would be better to find a way to put the extension in IR instead of in the assembler. For example, maybe the IR could look like (pseudo-IR):
zext i64 (trunc i32 (sub i64 (ptrtoint i64 @label2)), (ptrtoint i64 @label1))))
So that would slot into the constant inititalizer, and the backend would know to emit that pattern as:

.long 0
.long label2 - label1

Now that I've written it out, it seems worse and more complex. Let's go with .quad I guess.

stack size -> stack frame size

In D104060#2831605, @ellis wrote:

__llvm_mipmap section should have type ELF::SHF_NOTE so that it doesn't get stripped by --gc-sections.

In D104667#2831507, @nickdesaulniers wrote:

In D104667#2831505, @MaskRay wrote:

In D104667#2831488, @nickdesaulniers wrote:

GCC trunk: warning: the frame size of 80 bytes is larger than 0 bytes [-Wframe-larger-than=]
Clang trunk: warning: stack frame size of 88 bytes in function 'bar' [-Wframe-larger-than]

looks like the frontend doesn't use this particular text at all?

This diagnostic is emitted by clang (clang/lib/CodeGen/CodeGenAction.cpp).

-Wframe-larger-than= also records the module flag metadata "warn-stack-size" which can be useful for LTO.

% fclang -Wframe-larger-than=100 a.c  -o x          
a.c:1:5: warning: stack frame size of 888 bytes in function 'main' [-Wframe-larger-than]
int main(){
    ^
1 warning generated.
% fclang -Wframe-larger-than=100 -flto=thin -fuse-ld=lld a.c -o x
ld.lld: warning: stack size limit exceeded (888) in main

Can we try to make the FE match what's printed during LTO? It's odd that they differ, IMO. Also, GCC prints the threshold value, as does our backend; but the frontend still does not.

module attr

Agree that a function attribute is more appropriate considering the LTO behavior.

In D104667#2831488, @nickdesaulniers wrote:

GCC trunk: warning: the frame size of 80 bytes is larger than 0 bytes [-Wframe-larger-than=]
Clang trunk: warning: stack frame size of 88 bytes in function 'bar' [-Wframe-larger-than]

looks like the frontend doesn't use this particular text at all?

Looks great!

LGTM.

LG

(This was why I suggested we waited a bit on GCC's response...)

make win64 work

In D104060#2827790, @ellis wrote:

I probably sound like a broken record, but we've spent a lot of time making sure the map section can be extracted correctly and that the raw section has no excess info. This is a major feature of MIP, the profile data and the function info are separated so that only the necessary data remains in the binary. The question is whether we should extend an existing pgi to support this feature or if MIP deserves to be its own framework. I do see the value in extending one of the many existing pgi to reduce duplicate work. My thoughts are that it would be too invasive to do everything we need; add one or two new sections, create a new .profraw and .profmap format, add a few flags, and extend the tools. By keeping MIP separate, we can make design decisions that align with our code size goal that may not be so easy to do in existing frameworks.

update

Thanks for review:) I feel that I understand value profiling better now.

minor adjustment to comment and test

switch to the NS==0 approach after seeing See my prior comment -- your previous revision is sound and LGTM. If we go with this version, we do need to examine whether static allocation is on or not. Sorry for the trouble.
(did not notice in my previous test update...)

fix condition and improve test

Use ValuesVar instead of NS

improve a comment

add test

I think the main problem is when the value is negative. How to represent it?

Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

@marxin FYI the GNU-style function attribute no_profile

In D104475#2825841, @nickdesaulniers wrote:

In D104475#2825795, @MaskRay wrote:

In D104475#2825772, @nickdesaulniers wrote:

In D104475#2825711, @MaskRay wrote:

So if we don't want to offer guarantee for IR/C/C++ attributes, we can document that users may need to additionally specify __attribute__((noinline)) to suppress inlining.

I don't generally like that approach:

If a guarantee would cause inferior optimization or we cannot think of all the complication/fallout initially, I'd prefer that we keep the initial semantics narrow.

The guarantee is more important than the optimization.

If we cannot make a promise, don't provide a promise.
No-suppressing inlining makes the attribute freely composable with other attributes (https://lists.llvm.org/pipermail/llvm-dev/2021-April/150062.html )

IIUC, isn't @jdoerfert arguing that optnone and noinline being composable is _bad_?

The main challenge is storing the offset to the profile data without using dynamic relocations. This is complicated by the fact that we use comdat sections within the llvm_mipraw section and that ELF does not seem to directly support section relative addresses. The solution is to use PC relative relocations. start___llvm_mipraw-.Lref gives us the PC relative offset to the start of the raw section and _Z3foov$RAW-.Lref gives us the offset to the profile data for this function relative to the same PC. After we extract the map section, we can subtract these to get the value we want, the section relative raw profile data offset.

(_Z3foov$RAW-.Lref) - (start_llvm_mipraw-.Lref) = _Z3foov$RAW - start_llvm_mipraw

Some email conversations are not on Phabricator. I record a copy here so that people who are not subscribed can have a full view

Is dynamic linking fully functional?

A stage-2 -DLLVM_BUILD_INSTRMENTED=IR clang is 4.2% smaller (1-169620032/177066968) with this patch.

I need to add a test case, but this is the sketch.

In D103717#2826247, @nathanchance wrote:

This patch breaks a two stage instrumented build::

$ cmake \
    -B build/stage1 \
    -G Ninja \
    -Wno-dev \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_C_COMPILER=clang \
    -DCMAKE_CXX_COMPILER=clang++ \
    -DLLVM_CCACHE_BUILD=ON \
    -DLLVM_ENABLE_PROJECTS="clang;compiler-rt;lld" \
    -DLLVM_TARGETS_TO_BUILD=host \
    -DLLVM_USE_LINKER=lld \
    llvm

$ ninja -C build/stage1

$ export PATH=$PWD/build/stage1/bin:$PATH

$ cmake \
    -B build/stage2 \
    -G Ninja \
    -Wno-dev \
    -DCLANG_TABLEGEN=$(command -v clang-tblgen) \
    -DCMAKE_AR=$(command -v llvm-ar) \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_C_COMPILER=$(command -v clang) \
    -DCMAKE_CXX_COMPILER=$(command -v clang++) \
    -DCMAKE_RANLIB=$(command -v llvm-ranlib) \
    -DLLVM_BUILD_INSTRUMENTED=IR \
    -DLLVM_BUILD_RUNTIME=OFF \
    -DLLVM_ENABLE_ASSERTIONS=ON \
    -DLLVM_ENABLE_PROJECTS="clang;lld" \
    -DLLVM_ENABLE_TERMINFO=OFF \
    -DLLVM_ENABLE_WARNINGS=OFF \
    -DLLVM_TABLEGEN=$(command -v llvm-tblgen) \
    -DLLVM_TARGETS_TO_BUILD=host \
    -DLLVM_USE_LINKER=$(command -v ld.lld) \
    llvm

$ ninja -C build/stage2
...
ld.lld: error: relocation refers to a discarded section: __llvm_prf_data
>>> defined in utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced 232 more times

ld.lld: error: relocation refers to a discarded section: __llvm_prf_data
>>> defined in utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced by AsmWriterEmitter.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o:((anonymous namespace)::AsmWriterEmitter::run(llvm::raw_ostream&))
>>> referenced 8 more times
...

In D104475#2825772, @nickdesaulniers wrote:

In D104475#2825711, @MaskRay wrote:

So if we don't want to offer guarantee for IR/C/C++ attributes, we can document that users may need to additionally specify __attribute__((noinline)) to suppress inlining.

I don't generally like that approach:

In D104475#2825666, @melver wrote:

In D104475#2825297, @MaskRay wrote:

Should no_profile specify the inlining behavior? Though no_sanitize_* don't specify that, either.

I think it's somehow implicit, but I can't quite say how. There are some tests that check this, e.g.
compiler-rt/test/asan/TestCases/inline.cpp
[...]
noinstr does add noinline, but other uses of the attribute alone might not. But in the end inlining is a red herring, it just seems to be the easiest way to enforce the promised contract. See https://lore.kernel.org/lkml/CANpmjNNRz5OVKb6PE7K6GjfoGbht_ZhyPkNG9aD+KjNDzK7hGg@mail.gmail.com/
[...]

I think the previous Instrumentation/InstrProfiling.cpp changes can be considered stable now. So, ping:)

Should no_profile specify the inlining behavior? Though no_sanitize_* don't specify that, either.

Looks great! One nit

I think people's main question is what distinguishing features make MachineFunction instrumentation appealing.

riscv64-linux-gnu-as -march=rv64gc a.s does use 2-byte alignment, so I think this may be fine.

(If the idea is to do the test in a merged patch.)

Sorry for being late on reviews.

Ping:)

LGTM.

After modifying the commit description, use arc diff --verbatim --head=HEAD 'HEAD^' to update the Phabricator subject/summary.

Also, have you checked how symlinks/hardlinks behave? link.exe uses sys::fs::getUniqueID.

You may need a test using -force_load for different archives.