I'm currently testing this patch. Will be committing after all tests are done.

LGTM. Thanks.

Oh, so DAG vs DAG-NEXT, right?

LGTM. Thanks.

LGTM

LGTM

LGTM

Thanks, Dehao. This LGTM now. I'm not 100% sure about the code paths in *Dwarf*.cpp, but they look safe enough. The only concern there would be any other code that also calls getDiscriminator, in which case, perhaps we could sink the DWARF version check into getDiscriminator and have getDiscriminator return ErrorOr.

In D25132#559074, @danielcdh wrote:

Another option would be: make SampleProfileFile option global, and checks if that option is empty here to decide if discriminator pass is needed.

LGTM

LGTM.

LGTM.

LGTM. Thanks.

OK, so instead of looking at the number of samples collected at the call site, it looks at the number of calls the target had. But what if the target is called disproportionately more from one call site than the other?

Apologies for the delay.

In D23284#526389, @anemet wrote:

@dnovillo or @rsmith, can you please confirm if you agree that this new option -Rpass-with-hotness should not be part of R_group. R_group options enable/disable groups of remarks whereas this one is only modifying the text of the remarks. Thanks!

Thanks! LGTM.

Apologies for the delay, Dehao. Thanks for the description. That needs to be added to the code, so we have it for future reference. I suspect that several of the spots where I was confused and asked for comments are going to be good candidates to sprinkle that high-level description around.

Could you describe this in *much* more detail?

LGTM

Adding Dehao. ISTR something special about 0 counts in profiles, but I don't recall details. Dehao, this looks fine to me, but will it break anything for samplepgo?

Any changes in build time? I guess I'm surprised that removal operations are more frequent than lookups.

LGTM.

LGTM with a couple of small changes.

LGTM. Thanks.

LGTM

LGTM. Thanks for the testcase and description.

LGTM. Thanks!

Thanks for the test case.

A small test case, perhaps?

I agree with Igor's analysis. Let's first fix the correctness issue. Any compile-time problems that show up, can be fixed with a data structure change later. Igor, do you have any test cases that show the problem? Without a test case, it's harder to justify this change.

Sounds like a great plan. Thanks for doing this!

In D20501#437688, @dnovillo wrote:

Now, if the sample profiler depends on instcombine, we'd have to schedule it after instcombine, right?

Ah. I should'be read this one first :) Ignore my question on http://reviews.llvm.org/D17742, then.

Now, if the sample profiler depends on instcombine, we'd have to schedule it after instcombine, right?

OK, sure. What I meant is a testcase that motivates this change.

Would it be possible to get a test case where we miss discriminator placement if simplifycfg doesn't run beforehand?

Cool, thanks.

I was going to ask about a test case, but we already have test cases that cover this, right?

LGTM. Apologies for the delay.

I have started going over this patch, but my knowledge of this area is pretty spotty. I defer to Duncan for a better opinion on this.

This looks fine, but let's wait until Easwaran's inliner support lands before submitting this change.

Looks much simpler. Thanks!

Nothing here seems to change the format of the external file, but could you double check with the AutoFDO converter that we can still read Perf profiles?

LGTM

In D18468#383779, @davidxl wrote:

Is there a way to exclude header sample from the body samples?

Needs a test. Otherwise, LGTM. Thanks.

Just to make sure AutoFDO is still working, would you mind building it with your LLVM build to make sure that create_llvm_prof still works?

Nice cleanup! I think we can prevent breaking the AutoFDO tool by adding an overload.

Could you give me a bit more context? A C/C++ motivating example would be great. I think I follow what the intent is, but I'd like to make sure and leave it documented in the commit log.

LGTM. Thanks for the fix!

Looks fine to me, though I'm not an expert on MD.

LGTM.

Thanks for the new feature! LGTM with one minor nit.

I just started looking at this. Some early feedback so we can discuss whether my idea makes sense. Mostly, I'm thinking we want to support summary data in text profiles for testing and debugging. Does that make sense to you?

Nice. Thanks. The shortened names are indeed easier to read (in addition to the size savings).

I like the new way of specifying weighted input better. Thanks! This LGTM with a couple of changes below.

In D15124#302904, @lvoufo wrote:

Once this is in, please update lib/Transforms/IPO/SampleProfile.cpp. It does something similar.

I've just started looking at this, so these first few comments are mostly for my benefit to make sure I'm understanding this.

How is this going to interact with SamplePGO? Currently, I'm testing a change that adds the InlineHint attribute for functions that globally collected a fraction of samples greater than a given threshold. Likewise, for functions that fall below another threshold, they receive the Cold attribute.

LGTM. Thanks.

In D14720#290631, @slingn wrote:

Related question:

Is it by design that intstr profile count overflow is treated differently from sample profile count overflow?
Instr profile merging appears to skip records that would result in overflow and print a warning. Sample profile count overflow saturates and does not show a warning.