Extending yaml profile format with block hashes, which are used for stale
profile matching. To avoid duplication of the code, created a new class with a
collection of utilities for computing hashes.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Rebasing...
This diff doesn't change existing behavior and used only to facilitate diffs down the stack. Hence, not adding tests here but please let me know if you prefer some.
bolt/lib/Core/HashUtilities.cpp | ||
---|---|---|
135–139 | It shouldn't affect the matching, but will make hash stable wrt opcode value changes. |
bolt/include/bolt/Profile/ProfileYAMLMapping.h | ||
---|---|---|
129 | Does it have to be a required field? This change breaks internal tests utilizing yaml profile which obviously don't have this field. I tried using an Optional instead and everything worked. |
Addressing comments:
- block's hash is optional in yaml
- using MIB->getShortBranchOpcode
I agree that not relying on specific opcodes is a good idea. Maybe that could be a follow-up diff?
Regarding LLVM's Hashing.h, I dropped one method from the diff, as it is already implemented. Anything else you have in mind regarding the helpers?
The current state looks good to me. I think @maksfb is suggesting the use of ADT/Hashing helpers instead of rolling up our own hashing primitives but that could be in a follow-up diff IMHO.
I agree that not relying on specific opcodes is a good idea. Maybe that could be a follow-up diff?
I'll work on that as a follow-up.
Does it have to be a required field? This change breaks internal tests utilizing yaml profile which obviously don't have this field. I tried using an Optional instead and everything worked.