[PATCH] D87953: [xray] Function coverage groups

This revision was automatically updated to reflect the committed changes. Closed by commit rG6f7fbdd2857f: [xray] Function coverage groups (authored by ianlevesque). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87953/new/ https://reviews.llvm.org/D

[PATCH] D87953: [xray] Function coverage groups

ianlevesque updated this revision to Diff 294213. ianlevesque added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Add static function to the test case, update documentation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D87953: [xray] Function coverage groups

dberris accepted this revision. dberris added a comment. This revision is now accepted and ready to land. Okay, I think I understand the motivation a little bit more now (i.e. sampling seems to be a useful thing to do anyway, regardless of whether we have the other facilities). Repository: r

[PATCH] D87953: [xray] Function coverage groups

ianlevesque added a comment. I can update the docs @MaskRay, not a problem. I'll tweak the test a little too per your comment. @dberris I think if we wanted to control it in a non-'random' (the crc32 was chosen so it would be consistent even though it's not really predictable up front) way we'

[PATCH] D87953: [xray] Function coverage groups

dberris added a comment. There's two tensions I'm trying to resolve in my mind with this patch: - Is there a way to make this more deterministic, instead of using a random sampling mechanism based on the name? - This looks like it's a subset of a more general facility to group functions by "mod

[PATCH] D87953: [xray] Function coverage groups

MaskRay added a comment. I probably have asked too much... but it'd be nice if you also add some documentation to `llvm/docs/XRay*.rst` That helps users :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87953/new/ https://reviews.llvm.org/D87953 _

[PATCH] D87953: [xray] Function coverage groups

MaskRay added a comment. In D87953#2293162 , @ianlevesque wrote: > Thanks @MaskRay - I tried to answer that question in > https://reviews.llvm.org/D87953#2286430. At present we are deploying > instrumentation to an arbitrary subset of our application usi

[PATCH] D87953: [xray] Function coverage groups

ianlevesque added a comment. Thanks @MaskRay - I tried to answer that question in https://reviews.llvm.org/D87953#2286430. At present we are deploying instrumentation to an arbitrary subset of our application using the instruction threshold. I would like to make the selection of how many and wh

[PATCH] D87953: [xray] Function coverage groups

MaskRay added a comment. In D87953#2293024 , @kyulee wrote: > I think it looks good to me. @MaskRay Any further feedback on this? This looks good from my viewpoint. One thing remains unanswered (https://reviews.llvm.org/D87953#2284071) is how the overhea

[PATCH] D87953: [xray] Function coverage groups

kyulee added a comment. I think it looks good to me. @MaskRay Any further feedback on this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87953/new/ https://reviews.llvm.org/D87953 ___ cfe-commits mailin

[PATCH] D87953: [xray] Function coverage groups

ianlevesque updated this revision to Diff 293826. ianlevesque added a comment. Remove extraneous parameter validations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87953/new/ https://reviews.llvm.org/D87953 Files: clang/include/clang/Basic/Cod

[PATCH] D87953: [xray] Function coverage groups

MaskRay added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1136 + if (Opts.XRayTotalFunctionGroups < 1) { +const Arg *A = Args.getLastArg(OPT_fxray_function_groups); +Diags.Report(diag::err_drv_invalid_value) Errors here are not

[PATCH] D87953: [xray] Function coverage groups

ianlevesque added a comment. This is ready for another review, I think I addressed everything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87953/new/ https://reviews.llvm.org/D87953 ___ cfe-commits mai

[PATCH] D87953: [xray] Function coverage groups

ianlevesque updated this revision to Diff 293289. ianlevesque added a comment. const Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87953/new/ https://reviews.llvm.org/D87953 Files: clang/include/clang/Basic/CodeGenOptions.def clang/include/cla

[PATCH] D87953: [xray] Function coverage groups

ianlevesque added a comment. In D87953#2284071 , @MaskRay wrote: > How large the overhead is? This is somewhat surprising to me. It is the binary size overhead, not the runtime overhead, that we are limited on (when deployed on Android). Repository:

[PATCH] D87953: [xray] Function coverage groups

ianlevesque updated this revision to Diff 293278. ianlevesque added a comment. Address code review feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87953/new/ https://reviews.llvm.org/D87953 Files: clang/include/clang/Basic/CodeGenOptions.

[PATCH] D87953: [xray] Function coverage groups

ianlevesque marked 7 inline comments as done. ianlevesque added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:814 +auto FuncGroups = CGM.getCodeGenOpts().XRayTotalFunctionGroups; +if (FuncGroups > 1) { + auto FuncName = ArrayRef(CurFn->getName().

[PATCH] D87953: [xray] Function coverage groups

MaskRay added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:814 +auto FuncGroups = CGM.getCodeGenOpts().XRayTotalFunctionGroups; +if (FuncGroups > 1) { + auto FuncName = ArrayRef(CurFn->getName().bytes_begin(), kyulee wrote: > Ma

[PATCH] D87953: [xray] Function coverage groups

kyulee added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:814 +auto FuncGroups = CGM.getCodeGenOpts().XRayTotalFunctionGroups; +if (FuncGroups > 1) { + auto FuncName = ArrayRef(CurFn->getName().bytes_begin(), MaskRay wrote: > Fo

[PATCH] D87953: [xray] Function coverage groups

MaskRay added a comment. The idea seems fine. > By selecting different groups over time you could cover the entire > application incrementally with lower overhead than instrumenting the entire > application at once. How large the overhead is? This is somewhat surprising to me. =

[PATCH] D87953: [xray] Function coverage groups

ianlevesque added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:817 +CurFn->getName().bytes_end()); + auto Group = crc32(FuncName) % FuncGroups; + if (Group != CGM.getCodeGenOpts().XRaySelectedFunctionGroup &&

[PATCH] D87953: [xray] Function coverage groups

dberris added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:817 +CurFn->getName().bytes_end()); + auto Group = crc32(FuncName) % FuncGroups; + if (Group != CGM.getCodeGenOpts().XRaySelectedFunctionGroup && ---

[PATCH] D87953: [xray] Function coverage groups

ianlevesque created this revision. ianlevesque added reviewers: dberris, MaskRay, kyulee. Herald added subscribers: cfe-commits, dang. Herald added a project: clang. ianlevesque requested review of this revision. Add the ability to selectively instrument a subset of functions by dividing the func