[llvm] Implements PGOBBAddrMap in Object and ObjectYAML with tests [1/5] (PR #71750)

[Micah Weston via llvm-commits]

red1bluelost wrote:




> Haven't read the code yet, but this is my high-level comment:
> 
> > Alternatively, we could just place all new fields into BBAddrMap. This makes the encoding/decoding code simpler. It has the cost of changing some behavior in the original API. It also adds unnecessary fields for anyone wanting to just use the regular BBAddrMap.
> 
> We can distinguish between the ELF representation (SHT_LLVM_BB_ADDR_MAP) and the in-memory representation (`BBAddrMap` which is returned by `readBBAddrMap`). It's perfectly fine to add new fields to the ELF representation. This is why the `feature` and `version` attributes exist. This means we don't need to define a new section type and duplicate the encoding/decoding logic. However, it's a sensible idea to define a new `PGOBBAddrMap` class so we don't clobber the existing `BBAddrMap` with unnecessary fields. WDYT?

I see what you mean by sharing ELF section (SHT_LLVM_BB_ADDR_MAP) rather than having it separate (SHT_PGO_LLVM_BB_ADDR_MAP). We would use feature byte to indicate enabled analyses. I would not be opposed to doing this.

If we keep the two classes `PGOBBAddrMap` and `BBAddrMap`, I don't think that this would simplify the encoding/decoding logic that much. `PGOBBAddrMap` would be the same while `BBAddrMap` would either need to error if Feature is not zero or decode but ignore the extra data. I personally would lean towards having `BBAddrMap` ignore extra data since `PGO...` is a superset and I suspect ignoring could simplify 'trait design' complexity.

I'll give a try to combine and have BBAddrMap ignore extra data in the mean time.

https://github.com/llvm/llvm-project/pull/71750