I have a bit more review to do, but this is what I've found so far. The naming comments are just suggestions, but the digit separators' are actually an issue.

LGTM

I agree with Zach, this should be little_t and big_t. You should also add aligned_big_t and aligned_little_t.

Ah, I see. This seems fine.

What's the use case?

LGTM.

Thanks for the update. This still looks incorrect in that SANITIZER_NON_UNIQUE_TYPEINFO should be 1 on defined(__arm64__) && SANITIZER_IOS and 0 otherwise on SANITIZER_MAC.

Then I'm fine with the patch as is.

Should the +0 be +0x0 so that it's always a hex digit? Or is the +0 what the binutils already output?

In D58028#1402149, @marxin wrote:

I can if you want. Have you tested that on MacOS or iOS with:

-stdlib=libstdc++

?

I do not believe this change is correct for MacOS or iOS. The original bug does not reproduce on MacOS, and on arm64 iOS you always need to compare strings. Could you preserve the existing behavior for those platforms?

lgtm

Ah, the bugs canceled each other out. Adding a unit test is fine. Just add a new file in llvm/unittests/Object/.

Overall this looks fine, but can you add a test of something that this fixes? I'm rather surprised that no tests fail now.

In D43157#1283535, @PkmX wrote:

Ping?

I have a patch available that handles PCREL_LO12_S and is rebased on trunk, if it is okay I will create a separate patch for reviewing.

lgtm

I'm all in favor of removing the warnings. I was against adding them to begin with.

Pretty sure I understand the algorithm now, but I still want to see what performance impact it has to see if it's worth the (minor) complexity. I'll benchmark it on some large links and see what happens.

I really like adding the option to disable it, but I'm not sure about disabling it by default. Is there any situation where the profile is present where a user wouldn't want to perform layout? The algorithm is very fast so unexpected link time increase isn't really an issue, and the exact order of sections already isn't guaranteed by ELF and differs between linkers so I don't expect there to be correctness issues.

• Added test for unorderable symbols from object files
• Removed unneeded temporary

ping

Address review comments.

lgtm

lgtm.

This just needs the recommonmark python package added on whatever server builds the docs.

In D48859#1151697, @zturner wrote:

In D48859#1151696, @Bigcheese wrote:

Is there a good reason the use cases you want this for can't be added to libSupport? I'm a bit worried about continuing to add platform specific code outside of libSupport.

Mostly that it's quite large. See my recent post on llvm-dev about RFC: libtrace. I don't want to develop an entire library inside of libSupport, and something like a libtrace will unavoidably have a lot of platform specific code. As another example, consider lldb. There's no way to avoid a bunch of platform specific code, and it would be nice if LLVM had a solution that allowed LLDB to re-use some of LLVM's own platform specific code.

Is there a good reason the use cases you want this for can't be added to libSupport? I'm a bit worried about continuing to add platform specific code outside of libSupport.

lgtm

Remove duplication on updating Counts.

Add support for indirect calls using value profiling data.

• Move to new PM
• Use CallSite
• Bail early if no profile counts
• Use TargetTransformInfo::isLoweredToCall

I mean performance of actual cases. I'm interested in if the cache effects of list vs vector matter.

Sorry for the delay. What change in performance does this give?

Use the correct diff.

This is enabled by default as the time and size overhead is so small.

Address review comments.

Address review comments by adding const where applicable.

lgtm.

I've changed the behavior to better match the existing semantics.

• Remove IR parts, those will be in a separate patch.
• Update Extensions.rst
• Add assembly only test

In writing the documentation I ran into an issue defining the precise semantics of .cg_profile when it includes a symbol which has not previously been referenced. In an updated patch I'm treating it as if .weak <symbol> was written if <symbol> has not yet been used. This leads to the following behavior:

Adding some reviewers that last touched metadata.

The MC and Object parts are enough for lld, yes.

Previous update was missing tests. Fixed.

I'm also updating the lld patch that reads this, will have that up shortly.

Address review comments.

Addressed review comments.

Address review comments.

Address review comments.

Address review comments.

Address review comments.

Address review comments.

Address review comments.

Apply review comments.

Address review comments.

Rewrote the algorithm to match hfsort. Now gets the same performance as hfsort in Rafael's testcase.

In D43988#1024748, @ruiu wrote:

I don't think this is a right thing to do because creating a Windows executable doesn't necessarily mean that you are doing it on Command Prompt and vice versa. clang and lld support --rsp-quoting={windows,posix} to explicitly tell the driver how to parse response files. Didn't that work for you?

In D43988#1024762, @ruiu wrote:

LLVM's tools do not have a standard way to indicate the response file quoting style.

But if there was a problem that we had to fix by adding --rsp-quoting option, that will become a problem with this patch, no?

I'm not sure about this one. I've added Rafael as a reviewer as he's on the blame for most of this code.

This is good, but this should be using llvm-ar's failIfError instead of adding ExitOnError. Moving the error handling to ExitOnError should be a separate commit if done.

lgtm.

ping

ping

• Update to top of tree.
• Add some comments.

Seems fine and removes the duplication of names. My only concern is mixing --symbol-ordering-file with call graph sort in buildSectionOrder. Currently in the patch call graph sorting is applied then the order file. With this the two would need to be mixed into a single DenseMap, or the loop would need to be duplicated (which is what the patch currently does).

• Don't reorder non-reorderable sections
• Skip edges across output sections
• Add tests

• Order all sections and add a test
• Simplified
• Remove unused include

Changed to testcase from paper.

I've simplified the logic by getting rid of WorkLookup and just embedding the std::set iterator in Edge.

In D36351#957474, @ruiu wrote:

I wonder what was a motivation to implement two different ways to do one thing.

In D36351#957139, @ruiu wrote:

Then I wonder why you need to pass a profile data to the linker as a text file in the first place. Does the data embedded to an object file lack something, like inter-compilation-unit call frequency?

In D36351#956212, @ruiu wrote:

This needs to be implemented in the linker because of the line:

if (From.Size + To.Size > Target->PageSize)
  continue;

This is responsible for doubling the performance improvement on a game. Implementing this outside of the linker requires implementing most of the linker, including LTO.

And function sizes cannot be retrieved from the instrumented build (because instrumentation changes function size), so it must be computed at link-time. Is this correct?