This LGTM, please address hans' comments.

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.

LGTM

LGTM

Address reviewer comments:

The quadratic behavior leading to a 10 minute latency is too much. Binary search like you mentioned or just hash look up seems like it would drastically reduce the problem, could we bake it in the same change?

In D99487#2724702, @modimo wrote:

Agree with @MaskRay I'd like to see a holistics plan/RFC on all the pieces that are needed to get this functionality. The functionality here is only tested when exceptions are actually propagated which is not common in LLVM test suite. You'll want to run and likely write very specific tests stressing the test matrix here of BBS + MFS + EH. Given all that, a larger plan that details how we can be confident that all the pieces have been accounted for and will be well tested is (IMO) imperative in getting this change reviewed and accepted.

Address reviewer comments:

In D85085#2708898, @dblaikie wrote:

Thanks for this - tests generally look great. Code mostly makes sense to me (that's on me, not you) - couple of minor quibbles with comment wording, but nothing major I think.

Address reviewer comments:

In D99693#2706719, @dblaikie wrote:

In D99693#2706501, @aprantl wrote:

In D99693#2687996, @dblaikie wrote:

This patch does not affect C++ coroutines, since the DW_AT_specification is expected to hold the (original) linkage name.

So does this mean that for C++ we'll have a DW_TAG_subprogram with a DW_AT_linkage_name that doesn't match the symbol for that code in the object's symbol table?

Correct — and I would say this is probably a bug, unless we want DW_AT_linkage_name to show the "original" linkage name for purposes of matching breakpoints refering to the original name without the .resume suffix or something like that.

I'm a bit confused then.

since the DW_AT_specification is expected to hold the (original) linkage name.

What's this claim ^ about/based on? What happens if it doesn't hold the original linkage name?

@tmsriram - I think you hit a problem with debug info linkage names not matching the symbol names in the unique linkage name work. Do you recall what the problem was when those things diverged?

In D99487#2697789, @tmsriram wrote:

In D99487#2690528, @TaoPan wrote:

In D99487#2690162, @tmsriram wrote:

This is quite important to validate the correctness of the generated assembly. If you could please check that CFI and DebugInfo are sane before we take this patch forward? Let's get a temperature on how good these are before we decide how to take it forward.

Thanks! I got the importance.
As for DebugInfo, I tried to add "--basic-block-sections=all" to RUN of llvm/test/DebugInfo/COFF/comdat.ll, text sections includes basic block text sections and CodeViewDebugInfo can be created correctly after

1. apply this patch
2. commenting out a Linux exception handling protocol dependent function which I plan to do in later basic block section Windows exception handling patch.

There are some non-blocking errors "error: all .cv_loc directives for a function must be in the same section" in llc stage, one error for each basic block, below is the first one.
f.__part.1: # %if.then
#DEBUG_VALUE: f:c <- $ecx
<unknown>:0: error: all .cv_loc directives for a function must be in the same section
.Ltmp3:

Would you be able to paste the entire assembly with cv loc directives? I can try to take a look.

As for CFI, does it need be verified addition to llvm/test/DebugInfo/ tests? If so, could you please guide me how CFI of basic block sections is verified on Linux?

Basically, this means your code won't assemble with CFI and that is a serious limitation. This is similar to what we did for ELF. All CFI directives should be within a cfi_startproc cfi_endproc directive as part of a single section. When you fragment a function into multiple sections, this breaks. We solved this by adding a separate startproc endproc directive for each section.

I have some suggestions for you to better understand the problem:

• GCC supports function splitting with -freorder-blocks-and-partition. You can use GCC to split a function and see what it does to cv_loc directives.
• D79978 is what we did with ELF, my guess is it is very similar. You just need to add a startproc endproc to every function and duplicate all the information to compute Call Frame Address.

In D99487#2690528, @TaoPan wrote:

In D99487#2690162, @tmsriram wrote:

This is quite important to validate the correctness of the generated assembly. If you could please check that CFI and DebugInfo are sane before we take this patch forward? Let's get a temperature on how good these are before we decide how to take it forward.

Thanks! I got the importance.
As for DebugInfo, I tried to add "--basic-block-sections=all" to RUN of llvm/test/DebugInfo/COFF/comdat.ll, text sections includes basic block text sections and CodeViewDebugInfo can be created correctly after

1. apply this patch
2. commenting out a Linux exception handling protocol dependent function which I plan to do in later basic block section Windows exception handling patch.

There are some non-blocking errors "error: all .cv_loc directives for a function must be in the same section" in llc stage, one error for each basic block, below is the first one.
f.__part.1: # %if.then
#DEBUG_VALUE: f:c <- $ecx
<unknown>:0: error: all .cv_loc directives for a function must be in the same section
.Ltmp3:

In D99487#2688224, @TaoPan wrote:

In D99487#2687506, @tmsriram wrote:

If it is easier for you to break this up, you can present exception handling in a separate patch with a clear "TODO:" at the appropriate places. That should be alright and is also easier to review.

I added "TODO:" for constructing exception section and cold section later.

Great, please use the parent-child relationships to mark these patches.

I marked parent-child relationships.

• Could you also please expand the summary of this patch? I would write what extra was needed to port this from ELF to PE-COFF.

I expanded the summary, could you please have a review?

• Could you also talk about DebugInfo handling and CFI with PE-COFF and basic block sections? Are these format agnostic and have you verified that they do the right thing? These were the biggest challenges for us when doing this for ELF.

No, I didn't verify DebugInfo handling and CFI with PE-COFF and basic block sections.

In D99487#2670028, @TaoPan wrote:

Hi tmsriram, MaskRay,

I updated implementation and test. Could you please have a look?
As for multiple COMDAT seletion kinds of PE-COFF, is IMAGE_COMDAT_SELECT_NODUPLICATES wrong? Or need to set different COMDAT selection kind in some case?

Address reviewer comments on adding various test cases to make sure loc lists are generated the same way with and without basic block sections except when the ranges cross section boundaries.

LGTM and is a simple change to capture the fall through bit.

Address reviewer comments and refactor code as mentioned.

D96109 moved the adding of suffixes to clang front end and this will not be an issue any more with llvm created functions.

Address reviewer comment to use numeric substitution block.

In D96109#2583589, @dblaikie wrote:

Looks good to me - maybe give it a week or so in case other reviewers have some unresolved concerns.

Address reviewer comments:

Address reviewer comments:

Thanks for doing this! Much appreciated!

Fix a bug.

Removed the atttribute and directly added logic in ItaniumMangle.cpp to check if decl is internal linkage as per @dblaikie 's suggestion.

This patch update handles the following:

Address all reviewer comments.

Update patch.

Update test to check if the old metadata is retained and the new one is not duplicated.

Update patch.

Check if metadata already exists.

Could you please give more details about COFF versus ELF differences with regards to

In D93747#2488387, @dblaikie wrote:

Seems alright to me - I think we've hashed out the deeper issues (missing opportunity for C functions which could/should be addressed by moving the implementation to the frontend, where those C functions can be mangled and then use linkageName to give them the same AutoFDO opportunities as C++ functions) here and elsewhere - but for what it is, the patch makes sense. I'd probably say drop the flag - " check if rawLinkageName is set and only set it when it is not null. " was implemented and seems that addressed the debug info issue without an awkward tradeoff between AutoFDO fidelity and debugging fidelity, so there doesn't seem to be a need to be able to configure this.