Page MenuHomePhabricator

[LTO][lld] Add lto-pgo-warn-mismatch option
ClosedPublic

Authored by YolandaCY on Jun 16 2021, 6:32 PM.

Details

Summary

When enable CSPGO for ThinLTO, there are profile cfg mismatch warnings that will cause lld-link errors (with /WX)
due to source changes (e.g. #if code runs for profile generation but not for profile use)
To disable it we have to use an internal "/mllvm:-no-pgo-warn-mismatch" option.
In contrast clang uses option ”-Wno-backend-plugin“ to avoid such warnings and gcc has an explicit "-Wno-coverage-mismatch" option.

Add "lto-pgo-warn-mismatch" option to lld COFF/ELF to help turn on/off the profile mismatch warnings explicitly when build with ThinLTO and CSPGO.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
YolandaCY updated this revision to Diff 352646.Jun 17 2021, 2:02 AM

Fix default value in config and update the comment message

YolandaCY updated this revision to Diff 352651.Jun 17 2021, 2:36 AM

Reset previous commit

Fix default config value and option description

reset previous commit.

fix lto config default value and update option description

Could you help take a look?

Does the same issue exist on ELF?

@xur can you take a look - are these mismatches expected?

@YolandaCY could you just pass through the internal option? Not familiar with COFF syntax by for ELF with lld this is -Wl,-mllvm,-no-pgo-warn-mismatch. I assume COFF has a similar mechanism.

xur added a comment.Jun 21 2021, 12:05 PM

CSPGO can have mismatches -- it's just like PGO that is triggered by a source change, or a compiler change (before the pass).
Usually if there is a mismatch, PGO use pass will catch it first. There are cases that PGO has no-mismatch while CSPGO has a mismatch, but I consider it extremely rare.

Did you see the same warning in PGO?

As Teresa mentioned, I think the internal option should also work. But I do see the point of setting the argument in Conf to avoid the use of internal option.

@tejohnson Yes, I think the same issue exists for ELF. I just submit this for COFF to make sure the general process works, and may submit a seperate patch for ELF if necessary.
I verified the -mllvm or /mllvm option works for both COFF and ELF. Thank you for the suggestion!

@xur The mismatch happens for both PGO and CSPGO in my case. The PGO pass may catch it firstly when compile to object files, and then CSPGO will catch it again during post-link ThinLTO. We have root-caused that some of these mismatches are due to some specialization code when generate the profiles but disabled later for profile use.
Please find discussions on these warnings in Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=978401

Do you think we still need an explicit linker option to turn off the warnings (actually errors for lld link)?

A few high level comments/questions based on the discussion:

  • Is there a clang option? I thought there was but can't find one now. We should probably take the same approach here for the linker (external option vs use of the internal llvm option, similar name).
  • Since this is affecting regular PGO mismatches (via NoPGOWarnMismatch) and not just CSPGO the name should presumably not mention the "CS" part.
  • Please add for ELF as well in the same patch - should be straightfoward.
  • Tests needed.

We should probably settle the first question above first though - i.e. is there a clang driver option and if not, perhaps the internal option should be used via the linker as well.

Thanks Teresa for the comments.

For the first question, clang uses the -Wno-backend-plugin option to ignore the warnings.
This is mentioned in the How to build clang and LLVM with PGO user guide.

I haven't figured out how this correlates to the internal -no-pgo-warn-mismatch option though. Any tips?

Thanks Teresa for the comments.

For the first question, clang uses the -Wno-backend-plugin option to ignore the warnings.
This is mentioned in the How to build clang and LLVM with PGO user guide.

I haven't figured out how this correlates to the internal -no-pgo-warn-mismatch option though. Any tips?

I traced it through and it looks like it will disable any warning coming from outside of clang (so LLVM) that doesn't have another type of handling in this switch statement:
http://llvm-cs.pcc.me.uk/tools/clang/lib/CodeGen/CodeGenAction.cpp#818

So it looks like a larger hammer because it could affect other warning types too.

@xur @davidxl do either of you know why we don't support something more fine grained in clang such as gcc's -Wno-coverage-mismatch?

ormris removed a subscriber: ormris.Jun 23 2021, 9:48 AM

Thanks Teresa for the comments.

For the first question, clang uses the -Wno-backend-plugin option to ignore the warnings.
This is mentioned in the How to build clang and LLVM with PGO user guide.

I haven't figured out how this correlates to the internal -no-pgo-warn-mismatch option though. Any tips?

I traced it through and it looks like it will disable any warning coming from outside of clang (so LLVM) that doesn't have another type of handling in this switch statement:
http://llvm-cs.pcc.me.uk/tools/clang/lib/CodeGen/CodeGenAction.cpp#818

So it looks like a larger hammer because it could affect other warning types too.

@xur @davidxl do either of you know why we don't support something more fine grained in clang such as gcc's -Wno-coverage-mismatch?

Thanks Teresa for the pointer.

The corresponding lld diagnostic handler can be found at:
http://llvm-cs.pcc.me.uk/tools/lld/Common/ErrorHandler.cpp#66
It is much simpler and does not distingush the kind. And it is initialized in the LTOLLVMContext:
http://llvm-cs.pcc.me.uk/include/llvm/LTO/Config.h#228

It seems that to add a similar option as clang we may need to add an lto specific DiagnosticHandlerFunction (not share lld's) and check for all diagnostic kinds as clang?
Or shall we just add a fine grained option similar to gcc?

Thanks Teresa for the comments.

For the first question, clang uses the -Wno-backend-plugin option to ignore the warnings.
This is mentioned in the How to build clang and LLVM with PGO user guide.

I haven't figured out how this correlates to the internal -no-pgo-warn-mismatch option though. Any tips?

I traced it through and it looks like it will disable any warning coming from outside of clang (so LLVM) that doesn't have another type of handling in this switch statement:
http://llvm-cs.pcc.me.uk/tools/clang/lib/CodeGen/CodeGenAction.cpp#818

So it looks like a larger hammer because it could affect other warning types too.

@xur @davidxl do either of you know why we don't support something more fine grained in clang such as gcc's -Wno-coverage-mismatch?

Thanks Teresa for the pointer.

The corresponding lld diagnostic handler can be found at:
http://llvm-cs.pcc.me.uk/tools/lld/Common/ErrorHandler.cpp#66
It is much simpler and does not distingush the kind. And it is initialized in the LTOLLVMContext:
http://llvm-cs.pcc.me.uk/include/llvm/LTO/Config.h#228

It seems that to add a similar option as clang we may need to add an lto specific DiagnosticHandlerFunction (not share lld's) and check for all diagnostic kinds as clang?
Or shall we just add a fine grained option similar to gcc?

Personally I find it reasonable to have a way to disable just these warnings. I do find it unfortunate that the support for disabling these warnings is and will still be at a different granularity in clang vs LTO.

@xur @davidxl ping on history for not supporting a finer grain option for disabling this warning via clang.

My feeling is that it would be fine to go ahead here but I'd prefer that you add support for the ELF side as well.

YolandaCY updated this revision to Diff 357691.Jul 9 2021, 8:07 PM

Add ELF support and tests

YolandaCY retitled this revision from [lld] Add lto-cspgo-warn-mismatch option for COFF to [lld] Add lto-pgo-warn-mismatch option.Jul 9 2021, 8:34 PM
YolandaCY edited the summary of this revision. (Show Details)

Personally I find it reasonable to have a way to disable just these warnings. I do find it unfortunate that the support for disabling these warnings is and will still be at a different granularity in clang vs LTO.

@xur @davidxl ping on history for not supporting a finer grain option for disabling this warning via clang.

My feeling is that it would be fine to go ahead here but I'd prefer that you add support for the ELF side as well.

Thank you Teresa for the suggestions. I have added the option for ELF and also some tests. Please help take a look.

tejohnson accepted this revision.Jul 26 2021, 3:35 PM

lgtm with a test fix noted below.

lld/test/COFF/thinlto-pgo-warn.ll
11 ↗(On Diff #357691)

I think it would be cleaner to do a FileCheck --implicit-check-not warning (possibly with an --allow-empty to get FileCheck to succeed). I don't think it is is typical to test for the absence of a pattern by checking for a FileCheck failure.

Ditto for ELF test.

This revision is now accepted and ready to land.Jul 26 2021, 3:35 PM
MaskRay added inline comments.Jul 26 2021, 3:48 PM
lld/test/ELF/lto/thinlto-pgo-warn.ll
3 ↗(On Diff #357691)

Newer lld/test/ELF tests use ;; for non-RUN-non-CHECK comments.

8 ↗(On Diff #357691)

Drop --plugin-opt=thinlto

--lto-O2

11 ↗(On Diff #357691)

ditto

YolandaCY updated this revision to Diff 361956.Jul 27 2021, 2:45 AM

update tests

YolandaCY marked 4 inline comments as done.Jul 27 2021, 2:51 AM

Thanks Teresa and Fangrui for the comments. I have revised the tests as suggested!

lld/test/COFF/thinlto-pgo-warn.ll
11 ↗(On Diff #357691)

Thank you Teresa for the hint. I have updated the negative test as suggested.

lld/test/ELF/lto/thinlto-pgo-warn.ll
3 ↗(On Diff #357691)

Thanks and updated the comments for ELF test with "::".

MaskRay added inline comments.Jul 27 2021, 7:53 PM
lld/test/COFF/thinlto-pgo-warn.ll
11 ↗(On Diff #357691)
lld/test/ELF/lto/thinlto-pgo-warn.ll
11 ↗(On Diff #361956)

ditto

YolandaCY marked 2 inline comments as done.

Amend negative check

YolandaCY added inline comments.Jul 28 2021, 12:55 AM
lld/test/COFF/thinlto-pgo-warn.ll
11 ↗(On Diff #357691)

Thank you @MaskRay for the reference. It seems I cannot use a check prefix as a check pattern.

I did not use "--implicit-check-no warning" directly as it looks to me a more strict test that negates all warnings.
In this case, would it be better to simply use CHECK-NOT ?
I checked that if I run with flag /lto-pgo-warn-mismatch without the CHECK-WARNING prefix, FILECHECK will catch the failure.

YolandaCY updated this revision to Diff 365184.Aug 9 2021, 7:57 AM

Update test as warning message changed after rebase

This revision was automatically updated to reflect the committed changes.

Hi, this is breaking my build:

[1860/2777] Linking CXX shared library lib/libLLVMLTO.so.14git
FAILED: lib/libLLVMLTO.so.14git
: && /usr/bin/clang++-13 -fPIC -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -g  -Wl,--gdb-index -Wl,-z,defs -Wl,-z,nodelete -fuse-ld=lld-13 -Wl,--color-diagnostics -shared -Wl,-soname,libLLVMLTO.so.14git -o lib/libLLVMLTO.so.14git lib/LTO/CMakeFiles/LLVMLTO.dir/Caching.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/LTO.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/LTOBackend.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/LTOModule.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/LTOCodeGenerator.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/SummaryBasedOptimizations.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/UpdateCompilerUsed.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/ThinLTOCodeGenerator.cpp.o  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMCodeGen.so.14git  lib/libLLVMExtensions.so.14git  lib/libLLVMPasses.so.14git  lib/libLLVMTarget.so.14git  lib/libLLVMObjCARCOpts.so.14git  lib/libLLVMipo.so.14git  lib/libLLVMBitWriter.so.14git  lib/libLLVMLinker.so.14git  lib/libLLVMScalarOpts.so.14git  lib/libLLVMAggressiveInstCombine.so.14git  lib/libLLVMInstCombine.so.14git  lib/libLLVMTransformUtils.so.14git  lib/libLLVMAnalysis.so.14git  lib/libLLVMObject.so.14git  lib/libLLVMBitReader.so.14git  lib/libLLVMMC.so.14git  lib/libLLVMCore.so.14git  lib/libLLVMBinaryFormat.so.14git  lib/libLLVMRemarks.so.14git  lib/libLLVMSupport.so.14git  -Wl,-rpath-link,/home/culrho01/llvm-project/build/lib && :
ld.lld-13: error: undefined symbol: NoPGOWarnMismatch
>>> referenced by LTOBackend.cpp:228 (/home/culrho01/llvm-project/llvm/lib/LTO/LTOBackend.cpp:228)
>>>               lib/LTO/CMakeFiles/LLVMLTO.dir/LTOBackend.cpp.o:(runNewPMPasses(llvm::lto::Config const&, llvm::Module&, llvm::TargetMachine*, unsigned int, bool, llvm::ModuleSummaryIndex*, llvm::ModuleSummaryIndex const*))
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I seems cannot reproduce the failure. I tried:

cmake -DLLVM_ENABLE_PROJECTS=mlir -DLLVM_ENABLE_ASSERTIONS=On -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/opt/llvm/ -DLLVM_TARGETS_TO_BUILD=X86;NVPTX;AMDGPU -DLLVM_BUILD_EXAMPLES=ON -G Ninja ../llvm

ninja -j10

Is it specific to mlir build?

Hi, this is breaking my build:

[1860/2777] Linking CXX shared library lib/libLLVMLTO.so.14git
FAILED: lib/libLLVMLTO.so.14git
: && /usr/bin/clang++-13 -fPIC -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -g  -Wl,--gdb-index -Wl,-z,defs -Wl,-z,nodelete -fuse-ld=lld-13 -Wl,--color-diagnostics -shared -Wl,-soname,libLLVMLTO.so.14git -o lib/libLLVMLTO.so.14git lib/LTO/CMakeFiles/LLVMLTO.dir/Caching.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/LTO.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/LTOBackend.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/LTOModule.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/LTOCodeGenerator.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/SummaryBasedOptimizations.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/UpdateCompilerUsed.cpp.o lib/LTO/CMakeFiles/LLVMLTO.dir/ThinLTOCodeGenerator.cpp.o  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMCodeGen.so.14git  lib/libLLVMExtensions.so.14git  lib/libLLVMPasses.so.14git  lib/libLLVMTarget.so.14git  lib/libLLVMObjCARCOpts.so.14git  lib/libLLVMipo.so.14git  lib/libLLVMBitWriter.so.14git  lib/libLLVMLinker.so.14git  lib/libLLVMScalarOpts.so.14git  lib/libLLVMAggressiveInstCombine.so.14git  lib/libLLVMInstCombine.so.14git  lib/libLLVMTransformUtils.so.14git  lib/libLLVMAnalysis.so.14git  lib/libLLVMObject.so.14git  lib/libLLVMBitReader.so.14git  lib/libLLVMMC.so.14git  lib/libLLVMCore.so.14git  lib/libLLVMBinaryFormat.so.14git  lib/libLLVMRemarks.so.14git  lib/libLLVMSupport.so.14git  -Wl,-rpath-link,/home/culrho01/llvm-project/build/lib && :
ld.lld-13: error: undefined symbol: NoPGOWarnMismatch
>>> referenced by LTOBackend.cpp:228 (/home/culrho01/llvm-project/llvm/lib/LTO/LTOBackend.cpp:228)
>>>               lib/LTO/CMakeFiles/LLVMLTO.dir/LTOBackend.cpp.o:(runNewPMPasses(llvm::lto::Config const&, llvm::Module&, llvm::TargetMachine*, unsigned int, bool, llvm::ModuleSummaryIndex*, llvm::ModuleSummaryIndex const*))
clang: error: linker command failed with exit code 1 (use -v to see invocation)
pengfei reopened this revision.Aug 11 2021, 1:28 AM

Thanks @c-rhodes for reporting this. I have reverted it.

This revision is now accepted and ready to land.Aug 11 2021, 1:28 AM

I seems cannot reproduce the failure. I tried:

cmake -DLLVM_ENABLE_PROJECTS=mlir -DLLVM_ENABLE_ASSERTIONS=On -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/opt/llvm/ -DLLVM_TARGETS_TO_BUILD=X86;NVPTX;AMDGPU -DLLVM_BUILD_EXAMPLES=ON -G Ninja ../llvm

ninja -j10

Is it specific to mlir build?

I'm not building MLIR, here's my cmake line if it helps: cmake -G Ninja -DCMAKE_C_COMPILER=clang-13 -DCMAKE_CXX_COMPILER=clang++-13 -DCMAKE_EXE_LINKER_FLAGS="-Wl,--gdb-index" -DCMAKE_SHARED_LINKER_FLAGS="-Wl,--gdb-index" -DCMAKE_MODULE_LINKER_FLAGS="-Wl,--gdb-index" -DLLVM_USE_LINKER=lld-13 -DLLVM_USE_SPLIT_DWARF=OFF -DLLVM_APPEND_VC_REV=OFF -DLLVM_CCACHE_BUILD=ON -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=Debug '-DLLVM_ENABLE_PROJECTS=llvm;clang' -DCMAKE_INSTALL_PREFIX=install -DLLVM_TARGETS_TO_BUILD="X86;AArch64;RISCV" ../llvm

@YolandaCY, Did you test the shared build with -DBUILD_SHARED_LIBS=ON ? Is it because NoPGOWarnMismatch is defined in PGO while used in LTO?

@YolandaCY, Did you test the shared build with -DBUILD_SHARED_LIBS=ON ? Is it because NoPGOWarnMismatch is defined in PGO while used in LTO?

Yes, I guess so. I did not use -DBUILD_SHARED_LIBS=ON yet. I will have a try. Thank you all for hints!

I met the same issue. It may be fixed by the following patch.

diff --git a/llvm/lib/LTO/CMakeLists.txt b/llvm/lib/LTO/CMakeLists.txt
index e6adfd43e7f9..bbe256b3327b 100644
--- a/llvm/lib/LTO/CMakeLists.txt
+++ b/llvm/lib/LTO/CMakeLists.txt
@@ -25,6 +25,7 @@ add_llvm_component_library(LLVMLTO
   Extensions
   IPO
   InstCombine
+  LLVMInstrumentation
   Linker
   MC
   ObjCARC
MaskRay updated this revision to Diff 365776.Aug 11 2021, 9:24 AM
MaskRay retitled this revision from [lld] Add lto-pgo-warn-mismatch option to [LTO][lld] Add lto-pgo-warn-mismatch option.
MaskRay edited the summary of this revision. (Show Details)

fix build. fix cl::opt namespace

MaskRay accepted this revision.Aug 11 2021, 9:30 AM
MaskRay added inline comments.
lld/test/ELF/lto/thinlto-pgo-warn.ll
8 ↗(On Diff #365672)

No need to use 1/2/3 suffixes for different stages of the same source file.

13 ↗(On Diff #365672)

The exact hash value isn't necessary. It will just add maintenance burden when this changes.

MaskRay updated this revision to Diff 365781.Aug 11 2021, 9:45 AM
MaskRay edited the summary of this revision. (Show Details)

rename tests and simplify

This revision was landed with ongoing or failed builds.Aug 11 2021, 9:46 AM
This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: rnk.Aug 11 2021, 11:52 AM
rnk added inline comments.
lld/test/COFF/pgo-warn-mismatch.ll
5

This is the first use of llvm-profdata in the lld test suite, and it is not already a declared dependency for the test suite. I will add it in a moment. Without declaring test dependencies, the test may fail if one simply runs "check-lld" in a fresh build directory.