Page MenuHomePhabricator

[lld] Add lto-pgo-warn-mismatch option
Needs ReviewPublic

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).
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 this "lto-pgo-warn-mismatch" option to lld to help turn on/off the profile mismatch warnings explicitly when build with ThinLTO and CSPGO.

Diff Detail

Event Timeline

YolandaCY created this revision.Jun 16 2021, 6:32 PM
YolandaCY requested review of this revision.Jun 16 2021, 6:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2021, 6:32 PM
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.Fri, Jul 9, 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.Fri, Jul 9, 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.