[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

Fri Aug 16 10:17:49 PDT 2019

paulkirth added a comment.

In D66324#1632653 <https://reviews.llvm.org/D66324#1632653>, @lebedev.ri wrote:

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

So this change extends and revises what is in D65300 <https://reviews.llvm.org/D65300>.  The diffs are somewhat concurrent. For example the backend changes rely on the frontend warning being available. i.e. diag::warn_profile_data_misexpect.  But yes the logic is mostly the same.

I'm also not terribly familiar with Phabricator, so maybe I should have uploaded a diff between my two revisions, rather than between the my changes and top of tree?

> Can D65300 <https://reviews.llvm.org/D65300> be re-implemented to just use this llvm code?

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.

In the end, I decided that it actually made sense for clang to handle the clang based instrumentation, and llvm to handle the IR/Sampling instrumentation.

> I'm also thinking the clang-misexpect *tool* should be a separate change,
>  it's misleading to have it here as it gives impression that it **has** to be used,
>  which i'm not sure is so?

Sure, I can update this so that the standalone tool is in a separate change. The thought was to consolidate all the related changes needed for the standalone tool, but I see your point.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66324/new/

https://reviews.llvm.org/D66324