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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
