Page MenuHomePhabricator

[ThinLTO/WPD] Fix index-based WPD for alias vtables
ClosedPublic

Authored by tejohnson on Dec 4 2019, 5:59 PM.

Details

Summary

A recent fix in D69452 fixed index based WPD in the presence of
available_externally vtables. It added a cast of the vtable def
summary to a GlobalVarSummary. However, in some cases one def may be an
alias, in which case we need to get the base object before casting,
otherwise we will crash.

Diff Detail

Event Timeline

tejohnson created this revision.Dec 4 2019, 5:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2019, 5:59 PM

I was able to successfully create an ThinLTO-built clang.exe with this patch. However when compiling with assertions on, I'm still getting this:

Assertion failed: P.VTableVI.getSummaryList().size() == 1 || llvm::count_if( P.VTableVI.getSummaryList(), [&](const std::unique_ptr<GlobalValueSummary> &Summary) { return GlobalValue::isExternalLinkage(Summary->linkage()); }) <= 1, file D:\llvm-project\llvm\lib\Transforms\IPO\WholeProgramDevirt.cpp, line 852

I simply built a 2-stage clang with the following cmd-line:

stage 1: cmake -GNinja ../llvm -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_LIBXML2=OFF -DLLVM_ENABLE_PROJECTS="clang;lld;llvm" -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_C_COMPILER="C:/Program Files/LLVM/bin/clang-cl.exe" -DCMAKE_CXX_COMPILER="C:/Program Files/LLVM/bin/clang-cl.exe" -DCMAKE_LINKER="C:/Program Files/LLVM/bin/lld-link.exe" -DLLVM_ENABLE_LLD=true -DLLVM_ENABLE_PDB=ON

stage 2: cmake -GNinja ../llvm -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_LIBXML2=OFF -DLLVM_ENABLE_PROJECTS="clang;lld;llvm" -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_C_COMPILER="../stage1/bin/clang-cl.exe" -DCMAKE_CXX_COMPILER="../stage1/bin/clang-cl.exe" -DCMAKE_LINKER="../stage1/bin/lld-link.exe" -DLLVM_ENABLE_LLD=true -DLLVM_ENABLE_PDB=ON -DLLVM_USE_CRT_RELEASE=MT -DCMAKE_CXX_FLAGS="-march=skylake-avx512 /GS- -Xclang -O3 -Xclang -fwhole-program-vtables -fstrict-aliasing" -DCMAKE_C_FLAGS="-march=skylake-avx512 /GS- -Xclang -O3 -Xclang -fwhole-program-vtables -fstrict-aliasing" -DLLVM_ENABLE_LTO=THIN -DLLVM_PARALLEL_LINK_JOBS=10

Let me know if you need more info.

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

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

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

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

This did work. We have two vftable aliases with external linkage, that's why assertion fires. They're both attached to comdat, so there are no link errors.
May be just remove assertion?

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

This did work. We have two vftable aliases with external linkage, that's why assertion fires. They're both attached to comdat, so there are no link errors.
May be just remove assertion?

This appears to be an unrelated issue then, and should probably be fixed in a separate patch. Can this one be approved and go in, and I'll make sure I can repro and then fix this one?

I've used this patch since you've published it, on different contexts, things look good from my user perspective.

llvm/test/ThinLTO/X86/Inputs/devirt_alias.ll
2

Do you think you can add a repro for creating this input file? It's not always trivial if changes are needed further down the line.

llvm/test/ThinLTO/X86/devirt_alias.ll
26

Same comment as for the other file above.

Ping @evgeny777 @steven_wu!
Do you think you can take a look at this please? It'd be nice if it could land before the 10.0 branch. Thanks!

This revision is now accepted and ready to land.Jan 13 2020, 10:17 AM
tejohnson marked an inline comment as done.Jan 13 2020, 1:09 PM
tejohnson added inline comments.
llvm/test/ThinLTO/X86/Inputs/devirt_alias.ll
2

I'm not really sure what to put, as it has been awhile but it looks like I cloned a different devirt test and modified it to fit the situation that triggered this bug.

This revision was automatically updated to reflect the committed changes.

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

This did work. We have two vftable aliases with external linkage, that's why assertion fires. They're both attached to comdat, so there are no link errors.
May be just remove assertion?

This appears to be an unrelated issue then, and should probably be fixed in a separate patch. Can this one be approved and go in, and I'll make sure I can repro and then fix this one?

Fix for assertion (remove it and add test) mailed in D72648.