Looks even better now!

LGTM, thanks for the fix!

Updating D84191: [llvm-objdump] Symbolize binary addresses for low-noisy asm diff.

In D84191#2212199, @MaskRay wrote:

The last update looks good. Let's wait a bit for binutils' comment.. I also have a question about whether we should name the option --symbolize-operands, or is there a better name. It drops the existing offset/address operand as well as adding a symbol.

In D84191#2210764, @MaskRay wrote:

I have seen a similar feature in GNU objdump. FWIW CCed you on https://sourceware.org/pipermail/binutils/2020-August/112803.html

Updating D84191: [llvm-objdump] Symbolize binary addresses for low-noisy asm diff.

@MaskRay How does this change look to you? Please let me know your comments. Thanks!

Updating D84191: [llvm-objdump] Symbolize binary addresses for low-noisy asm diff.

In D82822#2189692, @jdoerfert wrote:

In D82822#2189366, @hoyFB wrote:

In D82822#2187576, @jdoerfert wrote:

@davezarzycki The bots reported this as well, didn't build MLIR locally, this should have been fixed by 4d83aa4771d84940626d86c883193af390812281

I was seeing the same issue with what @davezarzycki has encountered. Not sure why this is related to MLIR. The failure is cause by this line:

74: // CHECK-DEBUG-NEXT:    [[OMP_GLOBAL_THREAD_NUM:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* **@12**), !dbg !{{[0-9]*}}

where @12 is supposed to be defined as

@11 = private unnamed_addr constant [90 x i8] c";llvm-project/clang/test/OpenMP/irbuilder_nested_parallel_for.c;parallel_for_1;85;1;;\00", align 1
@12 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([90 x i8], [90 x i8]* @11, i32 0, i32 0) }, align 8

The exact match of @12 seems a bit fragile to me since global data names could be changed from time to time. Can you please make it bit more robust? Thank

I just created D85099 to eliminate the problem permanently. Feel free to take a look.

In D82822#2187576, @jdoerfert wrote:

@davezarzycki The bots reported this as well, didn't build MLIR locally, this should have been fixed by 4d83aa4771d84940626d86c883193af390812281

In D84997#2186533, @wenlei wrote:

In D84997#2186518, @wmi wrote:

The multiple inline instance copies generated from loop unrolling or jump threading should have their own FunctionSamples objects. Why merging multiple inline instance to the outline profile will trigger assertion? Could you have a testcase for it?

It's a slightly difference case. In the training build, we didn't unroll, so there's a single inline site in the loop, but in the PGO optimizing build, the loop got unrolled with call site duplicated. And the duplicated call sites all point to the same FunctionSamples, in which case we should merge them only once. Agree that we should add a test case.

Updating D84997: [AutoFDO] Avoid merging inlinee samples multiple times

In D84997#2186553, @wmi wrote:

The multiple inline instance copies generated from loop unrolling or jump threading should have their own FunctionSamples objects. Why merging multiple inline instance to the outline profile will trigger assertion? Could you have a testcase for it?

It's a slightly difference case. In the training build, we didn't unroll, so there's a single inline site in the loop, but in the PGO optimizing build, the loop got unrolled with call site duplicated. And the duplicated call sites all point to the same FunctionSamples, in which case we should merge them only once. Agree that we should add a test case.

Thanks. So it is a combined version of normal PGO and sampleFDO, or the PGO here means the instrument hook for sampleFDO?

For the second problem, we got a bug report for the same problem. I create a testcase and a fix in https://reviews.llvm.org/D84994. I think your fix is more concise while mine is a straightforward fix. If you think your fix is better, free to use the testcase in D84994. Otherwise, I can commit that one.

I will commit Hongtao's version in D84994 so the two fixes can be separated.

In D84715#2179099, @weiwang wrote:

In D84715#2177627, @wenlei wrote:

I'm guessing these are not the only passes that will choke with empty/dead functions. If this is because ORE is being used for a special case, then we might want to fix the special case on the user side.

If we want passes to absorb such empty functions, then there needs to be consistency for all passes. I don't think passes need to absorb such cases though, as this is really a "limbo state" of functions when being removed (a normal empty function should still have entry block and ret instruction).

Agree. The proposed checks are only placed along the particular call path that leads to the failure. If we are going cover all the analyses/passes, the overall effort could be substantial.

If the big assumption is that there shouldn't be an empty body function to deal with, we can probably change how ORE is created at the beginning. Instead of creating a heavy-weight version of ORE (with all dependent analyses invoked), using the light-weight version OptimizationRemarkEmitter ORE(F, nullptr) at LTO.cpp:801.

In D80765#2177014, @dblaikie wrote:

In D80765#2174269, @MaskRay wrote:

In D80765#2174250, @dblaikie wrote:

In D80765#2174141, @hoyFB wrote:

In D80765#2170820, @dblaikie wrote:

Yeah - one alternative here would be for ThinLTO To "home" debug type information, the same way I believe it "homes" inline functions. (original/incoming IR to the thinlink may have multiple definitions of inline functions - one in every module that has a call to the inline function - but something in the think link summary basically says "drop this definition" in all but one of those modules and in that one blessed module it says "always produce a weak definition of this function, even if you don't need it" - something like that should be done with type information, which would remove the redundancy in the object files)

In the case of a full executable link (with the homing above implemented), I'd then suggest not enabling type units, since they just add overhead and wouldn't reduce any duplication - in the case of a static library built with ThinLTO (if that's even a thing), then you'd still potentially want to use type units so they could be deduplicated against other libraries.

Enabling compile-time type deduplication is indeed an ideal solution, as you pointed out finding a prevailing symbol definition and dropping others for duplicate functions from different modules. This would require quite some work in parsing type metadata on the IR and might slow down thinLink.

Fair point - though presumably the first stage compile would do the metadata walk through the IR, provide a list of types in the thin summary file and the thin link would then communicate to the backend compiles specifying which types each backend compile should emit into its respective object file.

Also this may not completely address deduplication of new data introduced during LTO postLink code generation time. The type unit level deduplication could be the last resort to handle the duplicates, like what is done in non-LTO build.

Not sure I follow - what duplication would remain if the thin link homed type descriptions? When linking to non-LTO (normal machine code/object files) libraries? Yeah, fair point (tangential warning: LLVM's type unit hashes are non-DWARF-conforming and not compatible with GCC's type unit hashes... :/)

It /sounds/ like, maybe the original fix in D62884 maybe was going in the wrong direction, and instead ThinLTO should be removing the comdat group from "homed" inline functions? Then there wouldn't be a need for a special case in how comdats are handled.

I am with @dblaikie here. A bit of archaeology finds that rG4de44b7ef87bcef83798eee69fdcbfab9866d52e was probably going in the wrong direction but it was indeed the simplest/least intrusive approach.
D62884 just made some refactorings. Just a thought: for ThinLTO backend, dropping comdat in processGlobalsForThinLTO may achieve some results.

Yeah, maybe something near/similar to https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/Utils/FunctionImportUtils.cpp#L275 - where the comdat is imported as available externally, and its comdat group is stripped. Theory is, maybe we should be stripping the comdat on the other side too.

Hmm, maybe not, though? What about non-whole-program-ThinLTO? There could be a comdat copy of this function in a static library that it should be deduplicated against?

In D80765#2170820, @dblaikie wrote:

Yeah - one alternative here would be for ThinLTO To "home" debug type information, the same way I believe it "homes" inline functions. (original/incoming IR to the thinlink may have multiple definitions of inline functions - one in every module that has a call to the inline function - but something in the think link summary basically says "drop this definition" in all but one of those modules and in that one blessed module it says "always produce a weak definition of this function, even if you don't need it" - something like that should be done with type information, which would remove the redundancy in the object files)

In the case of a full executable link (with the homing above implemented), I'd then suggest not enabling type units, since they just add overhead and wouldn't reduce any duplication - in the case of a static library built with ThinLTO (if that's even a thing), then you'd still potentially want to use type units so they could be deduplicated against other libraries.

@wmi We'd like to hear about your input on this. Thanks!

Updating D84191: [llvm-objdump] Symbolize binary addresses for low-noisy asm diff.

In D84191#2168738, @jhenderson wrote:

It looks like you might still have a binary file in the patch, that is no longer needed?

Updating D84191: [llvm-objdump] Symbolize binary addresses for low-noisy asm diff.

Updating D84191: [llvm-objdump] Symbolize binary addresses for low-noisy asm diff.

Updating D84191: [llvm-objdump] Symbolize binary addresses for low-noisy asm diff.

Updating D84191: [llvm-objdump] Symbolize binary addresses for low-noisy asm diff.

In D84191#2163668, @jhenderson wrote:

Could you post some examples of what the actual difference in output is, please? I don't quite follow from the tests/description what is intended to change as a result of this option.

Also, if you are adding a new option to llvm-objdump, please remember to document it in llvm/docs/CommandGuide/llvm-objdump.rst.

Updating D84191: [llvm-objdump] Symbolize binary addresses for low-noisy asm diff.

Updating D84191: [llvm-objdump] Symbolize binary addresses for low-noisy asm diff.

Updating D83187: [LoopUnroll] Update branch weight for remainder loop

Updating D83187: [LoopUnroll] Update branch weight for remainder loop

Updating D83187: [LoopUnroll] Update branch weight for remainder loop

In D69732#1771950, @ormris wrote:

I've done testing with the following global parameters.

• The base for the branch is llvmorg-10-init-8655-g94a4a2c97f8
• Used llvm, clang, lld, and llvm-ar from this branch.
• The sqlite kvtest program was the test payload.

This test compared an unmodified compiler from the base of the branch with a modified compiler with this patch applied and the loop optimisation passes mentioned above moved to the backend. The results were as follows. All numbers in seconds.

Run	Modified LTO	Modified SPGO+LTO	Unmodified SPGO+LTO
1	42.00	41.73	42.08
2	42.30	39.49	42.45
3	41.21	42.46	42.49
AVG:	41.84	41.23	42.34

TL;DR the average run using a compiler built with the modified SPGO pipeline is about a second faster. Definitely a positive initial result.

In D69732#1730732, @tejohnson wrote:

This probably needs to be taken over by someone who cares about full LTO performance (@wristow or @ormris ?). This patch was some cleanup of the full LTO sample PGO pipeline, but has a number of issues I enumerate in the summary.

Given the performance improvements here, I'd like to develop this patch further.

@fhahn I'm not sure who is an appropriate code reviewer. Can you please help find one? Thanks!

Thanks for the quick turnaround! I confirm the change fixes our build break.

Thanks for fixing these issues!

In D80765#2099946, @MaskRay wrote:

Apologies for my slowness getting to this patch.

symtab->ltoOutputComdatGroups does work:

% readelf -g a3.o.lto.o a3.o1.lto.o 

File: a3.o.lto.o

COMDAT group section [    4] `.group' [4068369915778327548] contains 2 sections:
   [Index]    Name
   [    5]   .debug_types
   [    6]   .rela.debug_types

File: a3.o1.lto.o

COMDAT group section [    4] `.group' [4068369915778327548] contains 2 sections:
   [Index]    Name
   [    5]   .debug_types
   [    6]   .rela.debug_types

The .debug_types from a3.o1.lto.o can be discarded by the new COMDAT logic. However, I am concerned that making it general as this patch does can miss some codegen bugs. See @pcc's argument in
in https://reviews.llvm.org/D56015#1339411 . Since .debug_types (notably, a non-SHF_ALLOC section) is the only COMDAT rule this patch will discard, how about special casing .debug_types (i.e. if isLTOOutput && the group is related to .debug_types)?

In D81981#2099825, @wmi wrote:

In D81981#2099769, @davidxl wrote:

The index format needs to be backward compatible, so there needs to be some version specific handling there (can be removed later).

I don't understand this part. Could you elaborate it -- why index format is different from raw format in backward compatiblity, and what is the version specific handling?

For function entry bb which have multiple successors, the existing algorithm in FuncPGOInstrumentation<Edge, BBInfo>::getInstrBB will insert the counter in all its successors. My current implementation simply adds a counter in entry block so in that case, it introduces redundent counter.

I can improve it by selecting a successor to not insert counter for it since it can be inferred from the counters surrounding it. With that implemented, I expect the profile size will be unchanged.

The current PGO instrumentation is based on MST.

Yes, it is based on two parts, selecting MST is one part and selecting src/dst node of each non-MST edge to instrument is another part.

Changing the instrumentation may require changes in how the counts of non-MST-edges are calculated (in PGOUseFunc::setInstrumentedCounts). So maybe adjust the MST to remove the sibling edge ?

The change to add entry BB as an instrumented BB is in function getInstrumentBBs which is shared by profile-gen and profile-use, so it will be consistent between profile-gen and profile-use. About adjusting MST to remove sibling edge, I feel it is inconsistent with current goal of MST selection. The goal of selecting MST is to avoid instrumenting the most frequent edges so we can minimize the cost. Removing a successor edge of the entry block is a different goal. Mixing these two goals will make things complicated. I feel it is simpler to add the change in the second part -- selecting between src and dst which node to instrument.

It's an interesting idea to improve the PGO profile quality with sample profiles. Thanks for working on this!

In D80406#2085563, @MaskRay wrote:

Hope @tejohnson can confirm this is good.

Updating D80406: [LLD][ThinLTO] Add --thinlto-single-module to allow compiling partial modules.

Updating D80406: [LLD][ThinLTO] Add --thinlto-single-module to allow compiling partial modules.

Addressed feedbacks.

Updating D80406: [LLD][ThinLTO] Add --thinlto-single-module to allow compiling partial modules.

Addressed feedbacks.

Updating D80406: [LLD][ThinLTO] A switch to allow compilation of only one module.

Rebased.

Updating D80406: [LLD][ThinLTO] A switch to allow compilation of only one module.

Updating D80406: [LLD][ThinLTO] A switch to allow compilation of only one module.

In D80406#2060728, @tejohnson wrote:

In D80406#2060623, @hoyFB wrote:

In D80406#2060565, @tejohnson wrote:

In D80406#2057639, @tejohnson wrote:

Can you beef up the testing? I.e. ensure that the save temps output files for just the specified module get created. Also please add a test to make sure this interacts well with distributed thinlto build options, i.e. -thinlto-index-only (e.g. that it creates the expected output files just for the specified module).

Not sure if you saw this comment from my earlier reply - please add additional testing as suggested above.

It's added. E.g, the not ls single1.o2 line checks only two intermediate objects are generated, one is the dummy ld-temp.o and the other has the symbol specified in the command line. The IDX checks are for thinlto index testing. Do you want me to add more? Thanks.

Oh I see now that a -thinlto-index-only invocation was added. What you should be testing for is the output file specific to the specified object. E.g. typically there is a *.thinlto.bc file emitted for each input object. We should get only the one corresponding to the specified module in this case.

Also the other test suggestion I had was to make sure that the save-temps output files work as expected (still get the ones corresponding to the specified module, and not for the other modules). There are a number of .bc files emitted for each object with save-temps but it would be fine to check for one of them I think.

In D80406#2060565, @tejohnson wrote:

In D80406#2057639, @tejohnson wrote:

Can you beef up the testing? I.e. ensure that the save temps output files for just the specified module get created. Also please add a test to make sure this interacts well with distributed thinlto build options, i.e. -thinlto-index-only (e.g. that it creates the expected output files just for the specified module).

Not sure if you saw this comment from my earlier reply - please add additional testing as suggested above.

Addressed feedbacks.

Updating D80406: [LLD][ThinLTO] A switch to allow compilation of only one module.

Updating D80406: [LLD][ThinLTO] A switch to allow compilation of only one module.

Address feedbacks.

Updating D80406: [LLD][ThinLTO] A switch to allow compilation of only one module.