[PATCH] D116180: [InstrProf] Add single byte coverage mode

Tue Jan 25 16:22:27 PST 2022

ellis added a comment.

In D116180#3270200 <https://reviews.llvm.org/D116180#3270200>, @phosek wrote:

> In D116180#3269931 <https://reviews.llvm.org/D116180#3269931>, @ellis wrote:
>
>> In D116180#3268478 <https://reviews.llvm.org/D116180#3268478>, @phosek wrote:
>>
>>> This change appears to be implementing `func-cov` mode proposed in https://groups.google.com/g/llvm-dev/c/r03Z6JoN7d4. Do you plan on implementing the support for `block-cov` and `func-cnt` modes as well? I'm asking because we're interested in the `block-cov` mode, but also because I'm somewhat concerned about the orthogonality of these modes in the implementation.
>>>
>>> For example, this change introduces the `VARIANT_MASK_BYTE_ENTRY_COVERAGE` flag to track whether the `func-cov` mode is used. This means that we're going to need two more bits for `block-cov` and `func-cnt` (three in total). Wouldn't it make more sense to instead have `VARIANT_MASK_BYTE_COVERAGE` and `VARIANT_MASK_ENTRY_COVERAGE` flags which could be combined to represent all four modes, requiring only two bits in total?
>>
>> I was only planning on adding `func-cov`. A while back I did a small experiment and found very little binary size difference between `block-cov` and full `block-counts`. This is because `block-counts` can instrument a subset of blocks and infer counts from the rest. With `block-cov` we cannot do this in the same way so we need to instrument every block. There might be a smarter way to infer some block coverage, but I don't know it.
>>
>> I decided to use just `VARIANT_MASK_BYTE_ENTRY_COVERAGE` for simplicity, but I'm open to using two flags like you suggested.
>>
>> Can I ask why you are interested in `block-cov` rather than using full block counters? If binary size is a concern then I would suggest inferring coverage from 8 bit counters, assuming overflow isn't a problem. If speed is the main concern then maybe `block-cov` is the right way to go.
>
> We're mostly concerned about performance so `block-cov` is still of interest to us, but this is a useful insight, thanks! If it's not too much of an overhead, I'd prefer using the two flags so we can introduce the `block-cov` mode later.

Sounds good! I'll use two flags but leave the other cases unimplemented for now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116180