This is an archive of the discontinued LLVM Phabricator instance.

Force check prof branch_weights consistency in SwitchInstProfUpdateWrapper
ClosedPublic

Authored by yrouban on Jul 2 2019, 12:02 AM.

Details

Summary

This patch turns on the prof branch_weights metadata consistency check in SwitchInstProfUpdateWrapper.
If this patch causes a failure then please before reverting do report the IR that hits the assertion and try identifying the pass that introduces the inconsistency. We have to fix all such passes.
See also the upcoming change D61179 in the Verifier.

Diff Detail

Repository
rL LLVM

Event Timeline

yrouban created this revision.Jul 2 2019, 12:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2019, 12:02 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

What types of testing has been done ? I assume the code base now is relatively clean so it is safe to turn this on by default?

What types of testing has been done ? I assume the code base now is relatively clean so it is safe to turn this on by default?

Frankly speaking I'm not 100% sure that the code is safe. There could be projects that might hit the assertion. E.g I have to fix one pass internally at Azul. I tested LLVM monorepo make check-all and our Azul Zing test suits. So, I believe that the only way to find issues is to land this check D64061 and the verifyer D61179 one by one with some reasonable interval (one week?).

let me do some internal PGO build tomorrow with the option on and get back.

my test hit some other unrelated debug assert. Can you first change the debug assert into error message so that there is no need to use assert enabled build to test the option?

yrouban updated this revision to Diff 207968.Jul 3 2019, 9:21 PM

Changed assert(false) to llvm_unreachable() for release builds to fail on inconsistencies. Though it seems to be too strict as the code can tolerate such inconsistencies.

please flip the flag to false by default and check in the rest first.

yrouban updated this revision to Diff 208111.Jul 4 2019, 9:13 PM
yrouban edited the summary of this revision. (Show Details)

extracted the preparation patch to the separate patch D64233.

please flip the flag to false by default and check in the rest first.

done. D64233 is landed.

davidxl accepted this revision.Jul 24 2019, 10:18 AM

I did some test with the patch with samplePGO and it works fine. LGTM

This revision is now accepted and ready to land.Jul 24 2019, 10:18 AM
This revision was automatically updated to reflect the committed changes.