In D67253#1670681, @lebedev.ri wrote:

Re concurrency - you had standalone LLVMContext for each thread, right?

In D67253#1670569, @lebedev.ri wrote:

Layering feels weird here.
Can this be implemented as/limited to just a
run-clang-misexpect.py wrapper over clang itself?
That would be best IMHO.

Addresses problems running the standalone tool w/ the libTooling executors.

Revert mismerge when landing.

In D66324#1665784, @lebedev.ri wrote:

In D66324#1665773, @gribozavr wrote:

Reverted in r371598.

Another concern that I have is cross-compilation. LLVM's ADT is not set up to be cross-compiled like the rest of compiler-rt is.

Uhm, i have a better question still - why xxhash is even there? it's not used in the diff, and was not reviewed, it just magically appeared in the last update of the patch:
https://reviews.llvm.org/D66324?vs=219617&id=219645

Add a new profdata file to use w/ misexpect-nonconst-switch.c

Improve diagnostic output with profile counts

Remove commented out code

Address code review

Add clang and LLVM tests for __builtin_unpredictable

Add inline checks for misexpect tags to LowerExpectIntrinstic test

Update LowerExpectIntrinsic/basic.ll to generate misexpect metadata for a switch

Fix comment for misexpect option

Remove extra include from CodeGenFunction.cpp

In D66324#1652364, @lebedev.ri wrote:

Trying to start reviewing this.
The llvm side of the patch is self contained; clang patch should be split into a dependent review.

Address Review items

Thanks for the review.

Actually address nits

Address nits

@leonardchan can you revise your review against the latest revision? It looks to me as though you might have reviewed one of the first diffs instead of what is the latest code. For example the files in clang-tools have not been part of this revision for some time. Also when I try to respond to the comment, it appears against the very first diff. I'm happy to address any of the nits that remain, but want to make sure that what you reviewed and given approval for is what will be submitted.

Add comment to clarify choice of operands and reasoning about profiling metadata

This is being abandoned in favor of D66324, which reimplements this logic completely in the LLVM backend.

Change check for minimum number of metadata operands.

Remove namespace qualifier & insert check for number of arguments

In D65300#1647746, @lebedev.ri wrote:

Please can you clarify hat's the current layout of these patches?
Is this patch required, or is it superseded by D66324 (and thus should be abandoned)?
I'd like to begin reviewing, but i don't understand where to start.

Removes unused constructor, reformat code, and remove braces

Revert to use of auto when extracting metadata

Address Code Review

Add Nick Desaulniers as an additional reviewer

Update LLVM based implementation

Fix missing context in prior diff

• Update CodeGenOptions to remove -fmisexpect
• Access the LLVMContext directly
• Add -line-tables-only for more accurate diagnostics

Remove frontend components of clang-misexpect in favor of backend implementations

So it is possible to handle this completely in the backed, but the diagnostic output is not fantastic when using clang based instrumentation. In particular, we would need to emit the diagnostic in LowerExpectIntrisic.cpp by checking if the branch weight metadata already exists, and processing it there before it is overwritten. For some reason emitting the diagnostic at that point will not also emit the helpful source listing if there is a macro, even though a FullSourceLoc is available. This isn't an issue for either the handling in SampleProfile.cpp or in PGOInstrumentation.cpp.

Remove reference to clang-misexpect from CMakeLists.txt

Removes standalone clang-misexpect from revision

In D66324#1632653, @lebedev.ri wrote:

This is marked as child revision of D65300 but it seems like they both add
the same logic, just into different components, D65300 to clang, this to llvm.
Is this duplication or are the diffs concurrent?

Use existing LLVM code for mapping case literals to their case arms.

Update threshold values to match those assigned when lowering __builtin_expect intrinsic.

Update documentation and fix typos

Address feedback from review

Fix typo in comments

Update diff to have proper context on Phabricator

Add missing test for switch statements when the expected value is not a compile time constant.

In D65300#1601733, @xbolva00 wrote:

Nice work!

Refactors some debug code to be more centralized and cleans up some comments.

I've added support to Syringe for dispatching injected behaviors using direct calls, which follows the advice of Kostya Serebryany on CFE-dev http://lists.llvm.org/pipermail/cfe-dev/2018-September/059312.html