[PATCH] D131306: [llvm][misexpect] Track provenance of branch weights

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added a comment.



In D131306#3756009 <https://reviews.llvm.org/D131306#3756009>, @tejohnson wrote:

> @davidxl @xur for review and thoughts.
>
> I'm a little wary of requiring that both pieces of metadata be carried together, as that seems very brittle to maintain in the compiler. What would happen if the MD_expected didn't get propagated by some pass along with the MD_prof? I think you would get a false negative, which I suppose is better than a false positive. An alternative, that I guess would require more extensive changes, is to add an additional item to the "branch_weights" list (would need to be obviously distinguishable by type from the list of weights since that can be variable though).

Agreed. This isn't my preferred solution, but it seemed far less invasive than changing the format of profiling metadata. Originally, I had looked into adding a provenance field to the metadata. It required changes to every test that has branch weights, and I balked at submitting that. I'm also a little wary of making a heavily used metadata type larger. I guess there isn't a lot of difference, but the external metadata is optional, and we could remove it after checking is complete.

Unfortunately, I just don't think there is a clean solution here. Either we make an invasive change to the metadata format, or we deal w/ updating 2 pieces of metadata everywhere.  I'm just very unsure about which is the right tradeoff.

> Patch needs tests showing uses of the new metadata, and some documentation in LangRef (i.e. near https://llvm.org/docs/LangRef.html#prof-metadata).

Yes, testing and documentation is something I plan to improve, but I wanted to get some feedback on this approach before investing too heavily.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131306