@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.

Honestly, this looks weird (why 2 threads not 1 for example)? Also for anyone running lto on multicore machine results would be surprising at the very least.

I think that adding extra flag to GlobPattern::create was better idea. Just set it to false by default, so that GlobPattern will still act as glob matcher for all other users:

static static Expected<GlobPattern> create(StringRef Pat, bool IgnoreMetaChars = false);

Than add extra constructor to StringMatcher to which you'll pass GlobPattern and use it from InputSectionDescription constructor.

Thanks, I'm still in process of testing (now fixing issue which however is most likely related to devirtualization itself, not to this patch). Meanwhile some of my comments below.

I think this has to be rebased - I see multiple failures when trying to apply

LGTM

Ah, sorry, uploaded wrong file.

Addressed review comments

LGTM

Ping

Looks good at first sight. Do you have linker patch for me to try this out?

Could you possibly try check-all or check-llvm? This problem was occuring in the LLVM unit tests.

Strange this assertion probably means, that there are two summaries with external linkage in VTableVI. I tried reproducer out of curiosity, but it worked fine for me (no assertion on stage2 with ninja clang).

• Added test case for 'readonly' constants (import-ro-constant.ll)
• Added support for 'Constant' attribute to dot dumper
• Addressed other comments

Out of curiosity, what problem do you run into when you unify ReadOnly with Constant?

I reimplemented the patch, now using extra 'Constant' attribute in GlobalVarSummary.

Patch was tested on Firefox and LLVM test suites. Both pass flawlessly

Found a bug, will resubmit after fix and more thorough tests.

LGTM