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

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Jun 4 12:16:47 PDT 2022


MaskRay added a comment.
Herald added subscribers: Enna1, StephenFan.
Herald added a project: All.

In D116180#3276998 <https://reviews.llvm.org/D116180#3276998>, @kyulee wrote:

> LGTM. I'm approving this but suggest for other reviewers'. Also I left a few comments about naming counters that may follow up.

Thanks for contributing a new coverage mode. I believe it will be useful to some groups.
That said, process-wise, I think committing this just 6hr after this comment was probably too quick. 6hr did not give stakeholders enough time to read through a major new feature.
Some stakeholders may have other commitments so don't respond timely. But if you gave more time with the patch in an "approved" state, they would probably want to comment.

I think my main question is why does a coverage mode need to piggyback on `PGOInstrument.cpp` which is mainly for optimizations.

>From the functionality implemented by `-pgo-function-entry-coverage`, I don't see how it can be used for optimization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116180



More information about the llvm-commits mailing list