In D140478#4015568, @dblaikie wrote:

In D140478#4012779, @RamNalamothu wrote:

In D140478#4011085, @dblaikie wrote:

Is there any mechanism that tests whether a given range bounded by two labels may be subject to linker relaxation? It'd be nice to have that abstraction and use it here (& in some other places in DWARF emission) rather than hardcoding one particular architecture.

Unless the relaxations are performed during the assembling, this may not be possible before linking because we will not know which set of labels have instruction sequence(s) that the linker actually might be able to relax after finishing the section layout.
So, depending on whether linker relaxations are enabled, I think we can only either emit relocatable symbols for all the range labels or never.

Yeah, such a query wouldn't have to be precise, only conservatively correct - so if you can't be sure a given range won't be relaxed, then the query would say that it might be & DWARF would be emitted that'd be resilient to such relaxation.

The other place this would be needed is in the low/high_pc computation, since currently, even in DWARFv4, the high_pc is encoded as a delta from low_pc, not as a separate relocatable label, so that'd need the same checking.

I have introduced a new MCAsmInfo API to check if we need relocatable symbol references in dwarf entries based on whether the linker relaxations are enabled.

In D140478#4011149, @tmsriram wrote:

In D140478#4011085, @dblaikie wrote:

Is there any mechanism that tests whether a given range bounded by two labels may be subject to linker relaxation? It'd be nice to have that abstraction and use it here (& in some other places in DWARF emission) rather than hardcoding one particular architecture.

The other place this would be needed is in the low/high_pc computation, since currently, even in DWARFv4, the high_pc is encoded as a delta from low_pc, not as a separate relocatable label, so that'd need the same checking.

And I was thinking that PROPELLER needed/was considering doing some linker relaxation too, so could benefit from such an abstraction/hook/check - but maybe they didn't end up needing/using linker relaxation? (@tmsriram - did PROPELLER ever end up needing/using linker relaxation? is that still something under consideration?)

Propeller needs linker relaxation for branch instructions. Making it a shorter branch when the offset can fit in one byte and flipping branches, from like je to jne to convert and remove a subsequent fallthrough.

I remember dealing with issues regarding low_pc, high_pc. I think you reviewed these patches, I forget the details now and can take another look to see how this can help.

Hmm, yeah, I'm confused - because I'd expect the code for ranges to have to handle this but judging by the code being added in this patch, the code isn't already there that I'd expect to be there for linker relaxation in PROPELLER.

Could you point me to the reviews and/or a simple-ish example that would reproduce the relaxation behavior for PROPELLER so I can see how the DWARF is emitted/works uot?

In D140478#4011085, @dblaikie wrote:

Is there any mechanism that tests whether a given range bounded by two labels may be subject to linker relaxation? It'd be nice to have that abstraction and use it here (& in some other places in DWARF emission) rather than hardcoding one particular architecture.

The other place this would be needed is in the low/high_pc computation, since currently, even in DWARFv4, the high_pc is encoded as a delta from low_pc, not as a separate relocatable label, so that'd need the same checking.

And I was thinking that PROPELLER needed/was considering doing some linker relaxation too, so could benefit from such an abstraction/hook/check - but maybe they didn't end up needing/using linker relaxation? (@tmsriram - did PROPELLER ever end up needing/using linker relaxation? is that still something under consideration?)

Added more comments.

Add Propeller.cmake config

LGTM. Please make sure all reviewers have approved the patch.

LGTM

LGTM

LGTM

LGTM, thanks for fixing this!

Delete forward declaration to BinaryBasicBlock.

clang -v of the failing command:
clang -cc1 version 11.0.0 based upon LLVM 11.0.0git default target
x86_64-unknown-linux-gnu

Splitting the changes.

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: