@dmgreen Thx, I've updated the diff.

Our case is a bit different. Given a 512M incremental flush threshold, I tested an LTO built that outputs a 5G bitcode file. The BackpatchWord is called 16,613,927 times, among which only 12 needs disk seek. Plus, each access visits 4-8 bytes on a page, and all visited pages are far away from each other. It is likely that the pages are not cached, and need to load anyway, and after a load, our code does not access enough data on a page to 'cancel' the page fault cost. So its cost could be very similar to seek.

The code in computeDeadSymbols will conservatively mark all copies live if any is. See not only the worklist iteration I modified here, but also the code in visit at lines 878-879, and the preserved GUID handling at lines 814-815. So if there is a collision, the colliding values may conservatively be marked live, so the loop in propagateAttributes will handle them. I.e they are either all dead or all live (especially after my fix here which will make the behavior more conservative in the chance case of any alias values that collide with a value marked live during module summary building). Other than the corner case I'm fixing here, this should be a no-op in behavior for that loop in propagateAttributes, which was already skipping all dead values, and which does check if it is a GlobalVarSummary already.

Rebased/improved test case. @tejohnson Can you please look at this? I think that after D73851 compile time regressions are unlikely (at least I don't see them in LLVM test suite)

I made a smaller efficiency improvement (no measurable impact) to skip all summaries for a VI if the first copy is dead. I added an assert to ensure that all copies are dead if any is....

@arsenm

This LGTM, but why is the verifier figuring out how to do its own RPO?

Can anyone look at this, please? Thanks

Does this mean that we run UnreachableMachineBlockElimID twice?

How did you notice the problem?

LGTM

Well, this makes sense. Also I've encountered with cases when asm proc uses X18 is like a callee saved register which doesn't prevent shadow call stack runtime from working properly. Calling this off.

@efriedma From AArch64GenSubtargetInfo.inc:

This speeds up thin LTO link more than 10 times on Ryzen-9 3950X / Windows 10: from 30 mins to less than 3 mins

This needs to be rebased

@pcc

That case seems somewhat questionable to me. If the symbols are being exported, it is presumably for the purpose of allowing the symbols to be used outside of the defining DSO. But LTO visibility based optimizations could make any such use of the symbols unsafe. For example with WPD it's unsafe to derive outside of the defining DSO and with dead virtual function elimination it's unsafe to call virtual functions outside of the defining DSO.

Addressed comments

What is the goal for this option? Do you expect any users of this or this is for debugging WPD?

@pcc Just to clarify: I'm using this patch to identify problems with WPD (typically those are either invalid reinterpret_cast or open type hierarchy). I find it convenient to obtain a list of devirtualized functions using pass remarks, "undevirtualize" some of them relink and run. With attributes it seems to be much harder

There is already a way to do this, with the attribute [[clang::lto_visibility_public]]. Why do you need another one?

• Added support for index based WPD
• Minor improvements in test case

if we didn't summarize foo, then we would incorrectly perform a single impl devirt to bar, when we shouldn't have devirtualized at all.

This won't work for the index-only WPD.

This can be addressed at the moment, because index based WPD supports only single-impl devirtualization. In this case we have function name in the index.

Added check for temp file

@tejohnson

This should happen if the type ID has no use on type metadata, but then I think it should be removed by WPD.

Not sure about strict aliasing, but adding -fwhole-program-vtables seems like a good idea. I'd also add -flto-visibility-public-std.

@tejohnson I think, I found the reason of crash, described in https://reviews.llvm.org/D73242#1893744. There is the following sequence of instructions in VirtualFileSystem.cpp.obj:

store i32 (...)** bitcast ({ [5 x i8*] }* @"??_7RealFile@?A0x1AE555D7@@6B@" to i32 (...)**), i32 (...)*** %1, align 8, !dbg !36936, !tbaa !15844
...
%3 = tail call i1 @llvm.type.test(i8* bitcast ({ [5 x i8*] }* @"??_7RealFile@?A0x1AE555D7@@6B@" to i8*), metadata !"1$f8264c715b0bffc7d68899cd8e73067d") #8, !dbg !36937
tail call void @llvm.assume(i1 %3) #8, !dbg !36937

The only users of VTable ??_7RealFile@?A0x1AE555D7@@6B@ are store instructions which is not reflected in summary index. This causes type.test not being eliminated during WPD and then lowering to false during LTT. So far we get unreachable in the linked image.

LGTM

Suggested: Support -save-temps for thin archives.

I'm seeing crashes with this patch in two stage compilation, reported here: https://reviews.llvm.org/D73242#1893744. However the change seems reasonable, so I think it makes sense to push it and then investigate remaining issues. That said, LGTM

I was thinking of addressing 'skip-module-by-distributed-backend' case. For instance see writeEmptyDistributedBuildOutputs:

if (SkipModule) {
  ModuleSummaryIndex Index(/*HaveGVs*/ false);
  Index.setSkipModuleByDistributedBackend();
  WriteIndexToFile(Index, OS, nullptr);
}

Addressed comments

LGTM

Thanks!

Rebased

I know I ran into issues when testing our apps with my changes that required this. But perhaps it is only the invocation at line 1180 - are you saying that one is needed but the others aren't?

LGTM

LGTM, thanks!

This patch is to be rebased against D73094

FYI we had to disable a related variable importing patch due to some widespread order of magnitude increases in thin link time (see D73724). Until that is addressed, this should go under an option off by default, or just held off.

This is an enabler for upcoming enhancements to indirect call promotion, for example streamlined promotion guard sequences that compare against vtable address instead of the target function

LGTM

Added check for regular LTO module in combined summary

LGTM, thanks!

LGTM

LGTM

Looks good so far. See remaining comment in D71907

Would it be worthwhile to add some checking in somewhere that there are no locals in the index read by opt?

Addressed

One issue with taking combined indexes in testing is the handling of module paths when there are locals (won't be hit by the included test which doesn't contain any). E.g. in WPD if we try to promote when the target is a local function, we attempt to get its promoted global name:

Removed WPD resolution from source index asm file.