[PATCH] D124481: [llvm][misexpect] Add tests for sample profiling

Fri May 27 10:03:01 PDT 2022

paulkirth added a comment.

So after looking at this for a while, I think the core issue is that MisExpect needs some kind of provenance information about whether or not the branch weight originates from an `llvm.expect` intrinsic. If we could know that, then MisExpect diagnostics should be correct even if you run several profiling passes or combine different types of profiling, since we would only report issues on branch weights that had the correct provenance.

I see two basic ways to do that:

1. Add a new field to MD_prof that carries the provenance
2. Add an external piece of metadata that describes the same thing

I think the best option is to simply add that information into the branch weight metadata directly, so add an extra field that is an enum or another piece of metadata that describes whether or not it comes from an expect intrinsic. That would avoid inventing any additional machinery to correctly update the branch weight //and// also some additional metadata that //may// be there.

Changing the layout of MD_prof seems to be problematic though, especially given that we need to maintain backwards compatibility. I think the normal way to transition these changes is to introduce a replacement metadata and use that everywhere instead of the old version. the bitcode reader can then automatically update the old MD -> the new one. This seems like it may be a lot to do, given that all LLVM tests that have branch weights would need to be updated (at least the FileCheck portions if nothing else). That can probably be done w/ a good regex, but its a large invasive change which is enough to make me hesitant.

@tejohnson what are your thoughts on this approach? Do you think this would be a good direction to pursue? Are there other factors that I've failed to consider?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124481