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

Mon Aug 29 10:51:25 PDT 2022

tejohnson added a comment.

In D131306#3756074 <https://reviews.llvm.org/D131306#3756074>, @paulkirth wrote:

> 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.

Well I was thinking the extra field would be optional as well and could be removed. But understood that this requires more changes (although maybe not if it is optional, and after your recent changes to centralize some of the prof metadata handling in the compiler).

> 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.

Ok understood. Hopefully others will chime in with feedback.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131306