lgtm

Please add a comment why copy is needed.

I am not sure what you try to achieve here. Basically, there is no need to to use environment variable.

Addressed Chandler's review feedbacks

lgtm

Tried the two patches with our internal benchmark -- the alignment related performance issue is still there. The problem disappears with -mllvm -x86-experimental-pref-loop-alignment=5 is used.

Can you send the full patch so that we can help evaluate it?

We verified that the regression we saw internally is due to loop alignment.

lgtm

performance tests with large benchmarks showed no regression.

Will test on some very large programs that are sensitive to icache pressure and get back.

convinced.

I will provide more comments on propagating the inline hint later. However, I do think it is wrong to propagate the cold attribute in inliner -- there should be already an inter-procedural attribute propagation pass that does this.

Addressed review feedback.

A follow up patch for a test is fine.

Is there a functional test case (i.e., better matching with this change)?

Update the fix.

got time to look at it more. The root cause of why the call resolves to a static variable in another module is from the following code in BitcodeWriter.cpp. The call to ::link was recorded to GUID: 14802282251123568682 which was correct. Since it does not have a value-id, the logic for sample PGO kicks in. 14802282251123568682 was treated as 'original guid' and the from the map, the PGO annotated GUID for the static var was found (note that one original guid maps to multiple GUIDs). After this, the call to :link now resolves to static var 'link's GUID.

lgtm

I will debug this a little more when I find the time.

lgtm

The relevant stack trace of the crash when invoking llvm-lto with -thinlto-action=import:

The patch itself is fine. The meta question is whether we expect this option to be generally useful?

This code looks fine to me. My next question is, is the pattern common? The amount of code and possible compile increase added is non trivial.

This change may affect instrumentation PGO. I suspect it will greatly increase count threshold which may negatively affect performance. For instance some not so hot targets have small function body which is always good candidate to inline (after ICP).

This looks fine to me. Is there any performance data with PGO?

Bottom up ininling can also create lots of cold inline instances. Other than the effect of blocking hotter callers from being inlined, current Machine Block Layout also has problems forming long hot traces leaving holes in code layout.

Does it penalize cases on the border line? For instance, if a 7 byte memcpy requires a byte copy loop of 7 iterations which is above the threshold. In this case, the runtime check will purely add runtime overhead. This is unlike more general partial inlining of stringop operations where we may have internal APIs to by pass similar checks inside the library function.

lgtm

Have we considered limiting the max number of peeled iterations instead of disabling it?

lgtm

Perhaps it is cleaner to

1. pass the information whether it is a full unroll to tryToUnroll
2. add a internal option FullUnrollAllowPeeling and make it off by default? This will be similar to UnrollAllowPeeling flag, but takes precedence if it is full unroll.

lgtm

The patch is missing documentation change.

Instr PGO has its own set of simplification passes, but still I think we should keep this patch NFC for instrPGO. If there are performance numbers that justify the InstrPGO pipeline changes, it can be discussed/done in a separate thread. This one should be left as SamplePGO only change.

Sorry about the noise -- I was juggling around several different issues :)

Why not putting this change into BranchProbablityInfo.cpp, so that other components can benefit from the more precise BP info?

Do you have more benchmark numbers? For reference, here is GCC does (for sandybridge and above) for mempcy when size profile data is available:

1. when the size is <= 24, use 8 byte copy loop or straightline code.
2. when size is is between 24 and 128, use rep movsq
3. when size is b above that, use libcall

lgtm

lgtm

Blindly downgrade the warning into remarks can be bad -- profile mismatch problems can go undetected. People may spend more time diagnosing performance regressions due to missed warnings. Worse, we may regress in compiler without noticing.

Added documentation.

Addressed review feedback.

lgtm

It feels weird that the error handling is the first parameter of the interfaces. Also is it possible to provide a default error handling ?

lgtm

lgtm

lgtm

The order is implementation defined which is deterministic -- and I am not sure about the annoyance part of it :) On the other hand, requiring everything related to output to have a fixed/well-defined order which is implementation independent can have its own cost in terms of compile time or memory usage.

lgtm

Limit NamedInstrProfRecord to the reader file seems ok -- but this class also looks like a useful convenience class. We can probably discuss that in a different thread if it is desirable.

The compilation output depends on 1) input source and 2) the compiler that produces it. When compiler changes, the output may change (though still semantically equivalent unless there is UB in the source). Changing the internal data structures such as hash functions basically changes the compiler, so the output is expected to have possible change. It is unrealistic to expect compiler behavior 'completely' fixed from version to version.

In summary, I think this patch should be minimized by

Instead of exposing Counter members widely (and fixing tests), it is better to add wrapper methods in InstrProfRecord to forward the call to the nested member struct.

No, I don't have other comments.

Looks like this thread has gone stale for a while.