Thanks for working on this. You mentioned

This patch changes the behavior of decodeBBAddrMap to trace through relocations to get correct function addresses if it is going through a relocatable object file."

How is this achieved? Each BBAddrMap section has a corresponding .rela section, but that .rela section does not contain real address information (the addresses are only known at linker time). (Most of the time) It has entries of <offset_into_bbaddrmap_section, ".text", addend>. Are you computing function addresses as ".text address as exhibited in the object file + addend"?

LGTM

LGTM

In D125426#3509715, @tmsriram wrote:

In D125426#3509699, @shenhan wrote:

Here is the proposed workflow to build a propeller optimized clang binary (similar to PGO build).

1. Build instrumented clang binary:

cmake -G Ninja ... -DLLVM_BUILD_INSTRUMENTED=BBLabels ... llvm-project/llvm
ninja build

2. Using the above built binary in day-to-day routine and collect perf.data file and then convert it to propeller profiles.

3. Build the optimized clang binary:

cmake -G Ninja ... -DLLVM_PROPELLER_PROFILES=<profile> ... llvm-project/llvm

The experiment data show propeller optimized clang has a 5% compiler time improvement over PGO build.

Thanks! Please move this to the description.

In D122930#3494676, @rahmanl wrote:

In D122930#3492643, @shenhan wrote:

This looks good to me. One question, do we have a rough idea, what is the percentage of functions that pgo and propeller disagree about their hotness?

Yes. For one example we have
31% of Propeller functions in .text.hot,
57% in .text
11% in .text.unlikely

Here is the proposed workflow to build a propeller optimized clang binary (similar to PGO build).

1. Build instrumented clang binary:

cmake -G Ninja ... -DLLVM_BUILD_INSTRUMENTED=BBLabels ... llvm-project/llvm
ninja build

This looks good to me. One question, do we have a rough idea, what is the percentage of functions that pgo and propeller disagree about their hotness?

In D112679#3094869, @rahmanl wrote:

In D112679#3094847, @shenhan wrote:

One question, is "BBAddrMap" (or "ELF_BBAddrMap" in the origin code) only used to carry return values? You mentioned the section size won't change (for both ELF32 and ELF64), is it because you do not use this structure for writing out to ELF section?

Correct. BBAddrMap is only used in the return type for decodeBBAddrMap. This is completely independent from how the entries are written out to the ELF section.

One question, is "BBAddrMap" (or "ELF_BBAddrMap" in the origin code) only used to carry return values? You mentioned the section size won't change (for both ELF32 and ELF64), is it because you do not use this structure for writing out to ELF section?

Thanks for making the change.

In D87121#2261176, @MaskRay wrote:

In D87121#2261061, @shenhan wrote:

Hi Maskray, thanks for the fix. Another latent concern is that apply the symbol ordering file unconditionally might cause unintentional consequence to the final binary to the extent that the binary might be corrupted or less optimized, in this sense, this patch might still be warranted (orthogonal to your CL), what do you think?

I think it is important for the user to specify the appropriate linker flags. None of the other flags (even similar ones, e.g. --dynamic-list, -T, --version-script) should have such an exclusion mechanism (one concern is that changing -o can change the behavior, which is not great). There may be someway for the mysql build system to not apply the linker options at config stage, or you should just hook the final link. For your case --no-warn-symbol-ordering can essentially nullify the time increase caused by --symbol-ordering-file.

Hi Maskray, thanks for the fix. Another latent concern is that apply the symbol ordering file unconditionally might cause unintentional consequence to the final binary to the extent that the binary might be corrupted or less optimized, in this sense, this patch might still be warranted (orthogonal to your CL), what do you think?

Hi Fangrui, I've put up a reproduce case for mysql here. These are steps:

In D85408#2253055, @MaskRay wrote:

I am still reading the patch, but I have noticed one thing worth discussing. .bb_addr_map is a non-SHF_ALLOC section (meaning that it is not part of the memory image). An absolute relocation type (.quad .Lfunc_begin0) works but the value is a link-time address, not taking account of the load base (PIE/shared object)). (If .bb_addr_map has the SHF_ALLOC flag, linkers will report a text relocation) How do you intend to use .bb_addr_map at runtime?

Hi, would you to take another look? Thanks, -Han

Thanks Rui. Uploaded the patch: 1) removed PropellerConfig 2). Removed PropellerCommon.h from patch (and moved it into llvm realm).

Fixed all variable names to comply with lld naming convention.

Hi Markray, thanks for the review.

Hello, over the past 2 months since our first draft of Propeller, we've made huge changes/improvements and this describes what happens -http://lists.llvm.org/pipermail/llvm-dev/2020-January/138426.html

Added lld/ELF/Propeller/README.md. Moved some of file comments into README.md.

Hi, we've refactored propeller and cleaned up dependencies and interfaces. Here are the change summaries:

Updated header file description. Also changed local variable names to lower case camel.

In D68062#1683618, @ruiu wrote:

Let me start from very high-level review comments before getting into the details of the actual code:

• First and foremost, can you add a file comments to each file to explain what you are doing in the file? It doesn't have to be detailed, but giving an overview at the beginning of a file is generally very useful.

Addressed maskray's comments.

@ruiu whatś your opinion on this?

Re-write the getAlignment function, keep the original logic but with the 3 additional tests:

Mind take another look?

@pcc thanks for the review. Updated the patch -

1. if getAlignment sees a ABS symbols, returns 0
2. initialize Ret to UINT64_MAX
3. in "addCopyRelSymbol", errors out if alignment is 0.
4. unit test case that tests for proper handling of ABS symbols.

Thanks @ruiu Updated the patch, also attached a test case.

@pcc do you agree with initializing Ret to uint64-1?

@pcc

Thanks Rui. I just updated the patch a little bit, that - st_value is only valid for alignment when symbol shndx is set to SHN_COMMON.

In D47396#1113043, @ruiu wrote:

+Han Shen <shenhan@google.com>

Han, did you try that idea before?

Thanks. (Comments revised and DefaoutOutSec -> Default)