[PATCH] D81682: [PGO] Extend the value profile buckets for mem op sizes.

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi marked 3 inline comments as done.
yamauchi added inline comments.


================
Comment at: compiler-rt/include/profile/InstrProfData.inc:839
+#if defined(_MSC_VER) && !defined(__clang__)
+
+#include <intrin.h>
----------------
davidxl wrote:
> There is __popcnt etc. Can they be used?
> 
> https://docs.microsoft.com/en-us/cpp/intrinsics/popcnt16-popcnt-popcnt64?view=vs-2019
Unfortunately, no. I already tried this. (note "popcnt and popcnt64 aren't available on arm/arm64 and not on all x86/x86-64 CPUs" in the older update message.)

I can confirm it on https://godbolt.org/z/bn5PEy


================
Comment at: compiler-rt/include/profile/InstrProfData.inc:842
+INSTR_PROF_VISIBILITY INSTR_PROF_INLINE
+int InstProfClzll(unsigned long long X) {
+  unsigned long LeadZeroIdx = 0;
----------------
davidxl wrote:
> Since these helpers are only used by runtime on target, not by the host compiler, they should be moved to InstrProfilingUtil.c instead as InstrProfData.Inc is shared by runtime and compiler.
InstrProfIsSingleValRange (hence InstProfPopcountll) is used by the compiler in lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp.


================
Comment at: compiler-rt/lib/profile/InstrProfilingValue.c:274
+COMPILER_RT_VISIBILITY void
+__llvm_profile_instrument_memop(uint64_t TargetValue, void *Data,
+                                uint32_t CounterIndex) {
----------------
davidxl wrote:
> Ideally, this function should be inline expanded by the compiler at instrumentation time -- but that can be done separately.
True.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81682