Found few issues with patch

Out of curiosity, what makes it look like it might be synthetic?

In D50066#1197101, @tejohnson wrote:

That new DITemplateValueParameter type doesn't look correct to me.

It turned out that the problem is more complex than it seemed initially, because this forward declaration causes metadata loader to do really horrible things. Here is how metadata of test/ThinLTO/X86/fwd-decl.ll looks after both modules are loaded by addRegularLTO:

Any comments? Teresa?

1. Now importing globals list from DICompileUnit in IRMover. This is needed because we may internalise GV in destination module, so it can reach final link.

I've updated the review with the full version. I've tested on few large applications and results are quite promising in terms of both performance and image size

That's true. Actually, thinking more about the above example - presumably your global analysis on these new const bits would have to look at the references on global variables, and treat that as a non-const ref and basically mark the referenced GUID as non-const, to represent the address as escaping, right?

My understanding of your patch is that there would be a 1-1 association between each bit in the RefAccessBits BitVector and ref edges on that same function

I think it would be cleaner/clearer to encode this info along with the Ref edges

Thanks for looking at it @tejohnson. Sorry I was too busy to respond earlier.
I've added extra test case to check function calls which can be constant folded.
The substCallExprArgument was changed to castCallResultIfNeeded which might sound more reasonable.

Any comments on this?

Addressed review comments

This and D45850 made 2 - 2.5% performance improvement to a large real world application already built with PGO + full LTO. Any chance to push this anytime soon?

LGTM with nit.

Unfortunately ConstantFoldLoadFromConstPtr seems to be useful, because it checks for GV initializer being constant and we're not forced by this constraint when evaluating static ctors.
However it turned out we can use ConstantFoldLoadThroughBitcast when evaluating both load and store.

Any comments on this?

Approved in the mailing list

Addressed review comments in the mailing list:

• added --initial-module flag to llvm-link
• test cases now use llvm-link instead of llvm-lto

Rebased + added @dblaikie

Addressed review comments

FYI: Here are some results from running LLVM test suite:

Added checks to test case

Any comments on this? @efriedma ?

Addressed review comments. The new version also handles scalar types which size is not power of 2.

Ok, I found an answer. Calling this off.

Yes, I've seen that as well, but it seems to me that type mapping logic has changed since then. @rafael ?

Addressed review comments

Addressed review comments

@tejohnson Can't we use ThinLTO.PrevailingModuleForGUID in the backend?

@grimar Can't internalization be skipped during backend processing rather than changing linkage in summary index?. At the very first glance it is possible to check if def is prevailing or not in MustPreserveGV (see thinLTOInternalizeModule)

ModuleId issue was also fixed

Changes:

• Var/function name StringRef is not saved for legacy summary formats (when VST is used instead of strtab).
• Now printing full module path in comment before cluster definition
• Added comments to every edge/node
• Added test case

@tejohnson The BM.parseModule is invoked in the very end of thin link (see runThinLTOBackendThread). How can it work if it's not guaranteed that we have valid BC data till this moment? Note: parse module uses Stream

At which point of the API is this guarantee?

Ping

Any comments or suggestions?

Addressed review comments

Also it shows linkage for functions and variables. For each function flag bits and number of instructions are shown as well.

I don't know about the best way to display this, could be color, bold, label, or other markers.