Open justincady opened 1 year ago
cc'ing some recent editors of InstrProfilingBuffer.c: @ellishg @petrhosek
Also, I am willing to work on/test/review this, but I will need some guidance. It is unclear to me where the authoritative layout of this data lives in code, and whether or not using the closure-based interface (VPDataReaderType
) is required.
I'm guessing you are following these docs? These APIs seem to be only supported for Clang-based instrumentation (using -fprofile-instr-generate
). I'm not super familiar with those bits so I could be wrong.
I came across this which seems to suggest value profiling isn't supported in this mode. Could you try -mllvm -disable-vp=true
and see if that works?
That being said, I do think it would be nice to support dumping raw profiles to a buffer with LLVM instrumentation. It might take some changes to the API to fully support value profiling, debug info correlation, and any other future profiling modes. I would be willing to collaborate on this work.
I'm guessing you are following these docs?
Yes, exactly.
These APIs seem to be only supported for Clang-based instrumentation (using -fprofile-instr-generate). I'm not super familiar with those bits so I could be wrong.
I'm even less familiar, but based on what I found so far I believe you are right. My filing this is more of a feature request than a defect. :)
I came across this which seems to suggest value profiling isn't supported in this mode. Could you try -mllvm -disable-vp=true and see if that works?
Thanks for finding that thread. And:
$ clang -fuse-ld=lld -mllvm -disable-vp=true -fprofile-generate instrprof-value-prof-real.c
$ ./a.out out.profraw
$ llvm-profdata show out.profraw
Instrumentation level: IR entry_first = 0
Total functions: 515
Maximum function count: 381184
Maximum internal block count: 893184
$
Wow. Ok, this issue is mistitled. The existing APIs do support IR-based instrumentation, but they do not support value profiling. Does that seem accurate?
This experiment tracks with the closest thing I found to someone implementing the request: this proposed patch for the Linux kernel. The patch never landed; it was rejected upstream. But, a significant portion of the patch is traversing the value profiling data (without actually using compiler-rt, which naturally would create maintenance issues). I didn't realize these two concepts (IR-based instrumentation, value profiling) could be separated.
That's correct, value profiling is a separate feature that's enabled by default for the IR-based instrumentation but can be disabled. The buffer interface doesn't support value profiling as far as I'm aware hence the issue you're seeing. I don't think there's any fundamental reason why it couldn't be supported, someone would just need to put in the effort. I'd love to see this happen and would be happy to help. @vns-mn and @xur-llvm might have additional context.
Value profiling is not enabled by default for front-end instrumentation, that is why it works without any option.
IIRC, the reason is that the buffer size for value profile data can not be determined a priori. Happy to review patches for this.
The profiling runtime APIs for freestanding environments do not consider value profiling data. Specifically, after a bit of investigation, I believe that:
__llvm_profile_get_size_for_buffer
does not consider the size of the value data__llvm_profile_write_buffer
does not gather and write the value data into the given bufferHere is a reproducer, adapted from
compiler-rt/test/profile/Inputs/instrprof-value-prof-real.c
:Using front-end instrumentation with the buffer-based APIs works correctly:
But IR instrumentation does not:
I don't know if the solution is a second set of APIs that do incorporate the additional data correctly or another approach would be preferred.