lgtm

lgtm

lgtm

ok, makes sense. Basically since the inner loop is laid out first, the outer loop can not individually split out the blocks already laid out. What is the manifest of the problem? wrong profile data or compiler crash or something else?

lgtm

Profile runtime has 4 ways of setting output: 1) default 2) compiler command line arg 3) environment variable at runtime; and 4) user invocation of runtime API at runtime. The order of the precedence is 4> 3 > 2 > 1. Is the behavior here consistent with that?

lgtm

Are the new PM inliner issues being looked into too?

Wait a few days in case others have comments.

Then why modifying existing tests at all? How about add an explicit test using i64 type?

Perhaps add a new test to demonstrate it (that i32 profile weight still works)?

lgtm

I like the direction of the patch. There is one concern -- it makes the IR not backward compatible which may affect some users. +vsk.

The reason is that the always inliner is still not in-par with regular inliner -- if you look at the IFI setup -- missing BFI information.

Perhaps also add clang option manual description.

Looks good. Makes the tsan and instrumentation interaction also cleaner.

lgtm

lgtm

I am ok adding an internal option to control this with it being default for now.

One major problem with this patch is that the AlwaysInliner is dummy, it is not aware of profile information thus won't be able to perform proper profile update after the inlining of always inline callees. This can be an issue for Frontend PGO or when thinLTO/LTO is on (where there is cross module calls to always inline functions -- though it is not common).

Is it possible to add a test case?

The AlwaysInliner does not operate on DAG, but depend on function order in the module, so it is possible to regress with the change. I don't have strong opinion on this, but you want want to hear others opinion.

What is the compile time implication?

lgtm

LGTM (if the option is documented, the documentation part also needs to be updated).

Adding a test to make sure they are in sync will be useful.

Change in this direction is welcome. Added vsk and beanz to comment on the implication on runtime builds.

Can you share the performance data ?

lgtm

This is in theory similar to profile guided size optimization -- the difference is that optnone may not result in size reduction.

For x86 target, should it be turned on when -fprofile-use= option is specified unless -fno-split-machine-function is specified?

I accidentally threw in a third choice :). Yes -fmemory-profile is fine
which is consistent with -fsanitize=

I am fine with -fmemory-profiler.

A heads up -- I won't be able to review patch until mid Sept. Hope this is fine.

looks good to me. Wait to see if Craig has any more comments.

lgtm

I suggest the following steps to commit this patch:

lgtm. The user option should be documented after the runtime is ready.

I think the user visible option needs to match the functionality. Internal naming just needs some comments to document.

I mean the check llvm::shouldOptimizeForSize(L->getHeader(), PSI, BFI,

PGSOQueryType::IRPass) on loop header.

one nit: since the same instrumentation can be used to profiling global variable accesses (especially those indirect accessed), the option name seems excluding those cases. Shall it be renamed to fmem-prof?

If shouldOptimizeForSize is not available for loop analysis, should the check of be removed unconditionally instead of gated upon whether there is a hint?

A lot of test changes can probably be extracted out as NFC (to make the string test more robust) -- this will reduce the size of the patch.

please also document the new commands in docs/CommandGuide/llvm-profdata.rst

lgtm

Regarding reroller -- compiler with PGO will adjust the agressiveness of the unroller based on instruction workset size estimation. Doing this in later pass or in Propeller can help catch cases that are mis-handled.

lgtm (as Eugene pointed out -- use std::accumulate instead ).

What is the use case of the new APIs?

lgtm

LGTM ( how much size savings can we expect?)

It is ok to push this patch, but please update the summary of this patch from https://reviews.llvm.org/D84766 plus the additional improvement bits. Also document the background history a little.

Looks good to me.

lgtm

lgtm

I think the test should use profile-gen option to compare generated hash of otherwise identical functions (except for memop).

lgtm

right. It occurred to me during review, but did not think of the hard coded
values in proftext depends on LE.

Just realized that we need a test case to show it fixes the original issue (existence with memop --> different hash). Ok as a follow up .

lgtm (with the small test enhancement)

lgtm

changes like in llvm/test/Transforms/PGOProfile/PR41279.ll etc can be independently committed.

Can you split out the test changes and commit it separately?

On version bump --- looks like FrontPGO has been bumping version for hashing changes while IR PGO had not, so it is ok to leave the version unchanged (current situation is also confusing as the version of IR and FE PGO are mixed).

We may also need to bump both the raw and index format version with this change. For the profile use side, we also need to keep the hashing scheme of the older version (in profile-use side). More details to come.

Rong has pointed out that having 2 bits may not be enough -- also instead of using | operator, add is better.

(probably better hold on the patch until after the timeout issue is resolved)

Cost: one or two additional section headers (.group section(s)): 64 or 128 bytes on ELF64.

thanks for the cleanup

thanks. Can you update the summary of the revert patch -- I think the first paragraph does not belong.

I think it is better to revert https://reviews.llvm.org/D68041 first with reasons listed, and then follow up with an improvement patch to reduce strtab usage.

Perhaps also keep setBlockFreq as a lower level interface? I am surprised that edge splitting is the only one user of it.

lgtm

lgtm

lgtm

lgtm

Inlining won't be helped unless there is a hot callsite to the all-zero count function -- but this should not exist. I think the major performance hit comes from 1) text.unlikely which may not be mlocked; and 2) all unbiased branches due to zero weights. So doing this depending it on entry count existence is fine, but we still to teach PGOUse to drop the body. I think a simpler design would be

The PGOUse pass can choose not to annotate any branches with total weights == 0. Now the question becomes how do we tell PGOUse pass whether the entry should be set to 0 or leave it not set. There are two ways to do it (to signal it is not really cold, but unknown):

The PGOUse pass can choose not to annotate any branches with total weights == 0. Now the question becomes how do we tell PGOUse pass whether the entry should be set to 0 or leave it not set. There are two ways to do it (to signal it is not really cold, but unknown):

1. Remove the function from the indexed format profile;
2. set all counts to some sentinel value such as -1.

If we don't need to handle all zero cases using scaling, we can remove the dependency on the always entry patch.