Thanks for making the changes, LGTM!

@wmi @davidxl FYI, we have an internal patch to address this issue further. This patch only scales up entry count of outline function for cold callsites, thus losing context-sensitiveness. But ideally, we could reuse the entire profile from that inline context, and merge it all back to the outlined function. We changed the function annotation/inlining order to be top-down, then as we decide to not inline a cold call site, we merge the entire FunctionSamples of that inlinee back to outline counterpart, and since we process functions in top-down order (ignoring recursion for a second), annotation of the outline callee is able to use post merge FunctionSamples which is more accurate than simple scaling of entry count. (Doing context-sensitive inlining top-down has other benefits too, as it allows specialization whiling inlining which helps maximize the benefit of having context sensitive profile)

Symbol list can be provided to llvm-profdata in a plain text file.

Thanks for making indexing available for extended binary format. We've recently adding indexing to binary format as well in an internal patch, and also observed similar (~30%) build time reduction for some large services.

Before:

...
  13.1: inlined callee: _ZN6StringD2Ev: 0, 0, 0 sampled lines
    No samples collected in the function's body
    Samples collected in inlined callsites {
      0: inlined callee: _ZN6String4freeEv: 0, 0, 6 sampled lines
        Samples collected in the function's body {
          2: 0
          4: 0
          5: 0
          6: 0
          7: 0
          8: 0
        }
        Samples collected in inlined callsites {
          6: inlined callee: my_free: 0, 0, 1 sampled lines
            Samples collected in the function's body {
              4: 0
            }
            No inlined callsites in this function
}
}
}

After:

...
  13.1: inlined callee: _ZN6StringD2Ev: 0, 0, 0 sampled lines
    No samples collected in the function's body
    Samples collected in inlined callsites {
      0: inlined callee: _ZN6String4freeEv: 0, 0, 6 sampled lines
        Samples collected in the function's body {
          2: 0
          4: 0
          5: 0
          6: 0
          7: 0
          8: 0
        }
        Samples collected in inlined callsites {
          6: inlined callee: my_free: 0, 0, 1 sampled lines
            Samples collected in the function's body {
              4: 0
            }
            No inlined callsites in this function
        }
    }
}

address review feedback

rebase

Add getSortedCallTargets and SortCallTargets, per review feedback.

Thanks for reply, @asbirlea. Now I see where the non-determinism comes from. I thought that BFI query APIs all go through a translation from BasicBlock* to BlockNode, thus in query APIs, we use BasicBlock* just as a look up key without actually dereferencing it. This is what makes me think removing basic block is fine (we'll have dead entry, but it won't be accessed). But if the dangling BasicBlock* pointer happens to point to newly allocated BasicBlock* later, the count we get for that new block can be non-deterministic. This is quite tricky.. I'm not sure how crash can happen still as it seems we don't dereference BasicBlock* in query APIs, though non-determinism is enough of a problem.

rebase

Thanks for the context, @chandlerc. Agreed that ideally BFI should be preserved by all loop passes. Not to defend this change, but I have a few questions just to make sure I understand the cause of miscompile/non-determinism and there's no other lurking issues around this.

update description

add message for assertion.

let GUIDToFuncNameMapper populate GUIDToFuncNameMap field of FunctionSamples.

avoid TLS and move GUIDToFuncNameMapper to class member

In D65848#1621227, @wmi wrote:

Thanks for working on the issue.

From reading the comment of LLVM_THREAD_LOCAL in include/llvm/Support/Compiler.h, it seems we should only use TLS for POD if we want it to be functioning as we expect on all platforms.

How about change GUIDToFuncNameMap and CurrentModule as member fields in class GUIDToFuncNameMapper so we can create them separately for each module? Add a unique_ptr of GUIDToFuncNameMapper in SampleProfileLoader and a pointer field of type "GUIDToFuncNameMapper *" for each FunctionSamples object, so FunctionSamples object can access GUIDToFuncNameMap and CurrentModule via the pointer of GUIDToFuncNameMapper. The initialization of GUIDToFuncNameMapper pointer of each FunctionSamples object can be done in GUIDToFuncNameMapper's constructor (need to pass Reader as a param to get all the FunctionSamples).

In D65060#1596177, @vsk wrote:

Thanks for explaining. I don't have any other concerns about the patch as-written, but am interested in how it fits in with LoopSink (per David's comment).

gentle ping.. @davidxl, let me know if the last update addressed your comments. thanks!

add hasProfileData check, expose tuning knob per review feedback.

address feedback

In D65060#1599421, @davidxl wrote:

It is also better to introduce a coldness factor parameter 'F', i.e, if the source block's count is less than 1/F of the header count, suppress the hoisting.

In D65060#1596194, @davidxl wrote:

Hal probably remembered more details. IIRC, the argument was that LICM provides IR canonicalization which can simplify analysis -- there were some examples about exposed vectorization opportunities etc.

In D65060#1596152, @davidxl wrote:

This was tried before. IIRC, the conclusion was to implement look sinking pass to undo non-profitable LICM. The loop sinking pass was in D22778. Can the loop sinking pass be enhanced to handle the case here ( I have not looked in details) ?

Address review feedbacks.

Extra note: we saw a case from one of the big services (at facebook) that's very similar to the example in LICM/sink.ll, where there're cold paths in the loop. And by checking block frequency to avoid hoisting to hotter blocks, we got good improvements (~7% for a CPU related metric).

@dblaikie, thanks for review. Could you help land this change? I don't have commit permission yet..

In D64784#1586900, @dblaikie wrote:

Test coverage? (you could add another inlined call to the test case you added for the feature - but just leave the inlined-at location with a zero column for that particular call - so you can still test both positive/negatiev in the one file)

Tweak test case to add coverage for zero-coloumn inline site

In D64033#1586625, @dblaikie wrote:

What's the motivation for this?

In D64033#1580985, @JDevlieghere wrote:

LGTM. Thank you!

In D64033#1578894, @probinson wrote:

If the change from DW_AT_MIPS_linkage_name to DW_AT_linkage_name made you think the older .o files were MIPS, more likely it is a change in the default DWARF version; older versions of DWARF did not have DW_AT_linkage_name.

I think the .ll tests are sufficient to show we emit the call_column so no need to update the odr-member-functions tests IMO.

Undo changes to odr-member-functions test.

In D64033#1577463, @JDevlieghere wrote:

In D64033#1577390, @probinson wrote:

I guess we can leave the .s file tests alone. Most of them look like they were derived from real -S output and it would be pedantically consistent to update them, but they don't have to be.
@JDevlieghere once you sort out the odr-member-functions the patch is fine with me. I am mildly curious why all three .o files got smaller.

I applied the patch and that obviously doesn't include the binaries, my bad. Do we need to regenerate these binaries at all? I'm tempted to just leave this test as-is.

@JDevlieghere once you sort out the odr-member-functions the patch is fine with me. I am mildly curious why all three .o files got smaller.

In D64033#1566445, @probinson wrote:

I suspect there are more tests that must or should be updated; grepping for "DW_AT_call_" turns up for example:
llvm/test/DebugInfo/NVPTX/debug-info.ll
llvm/test/DebugInfo/X86/dbg-value-inlined-parameter.ll
llvm/test/tools/dsymutil/odr-member-functions.cpp

and there are a variety of .s files with hand-generated DWARF that probably should be updated as well, under llvm/test/tools/llvm-dwarfdump.

Update more tests.

In D38061#1484486, @dblaikie wrote:

Seems like the right thing would be for the DWARF code that wants a rendered type name to pass its own printing policy, rather than changing some relatively global one.

(though also I have my doubts about the whole approach - macro expansion can change the line number as well as the column number, so only suppressing column number would be insufficient - and this also reduces the usability for users (because the file/line/column of an anonymous type is a useful debugging aid). Seems to me like this should be opt-in if it's supported at all - though ideally build systems would use -frewrite-includes rather than full preprocessing, and then macros and lines/columns would be preserved, I think)