[PATCH] LLVM changes for value profiling support

[Xinliang David Li]

On Thu, Jun 11, 2015 at 8:29 AM, Diego Novillo <dnovillo at google.com> wrote:

> This looks generally OK to me.  At a high-level:
>
> - I'm not sure what to do wrt old versions. We can keep accumulating back
> compatibility as the format evolves, but at some point we'll want to stop.
> - The patch needs IR tests.
>
> Some detailed notes and questions below.
>
>
> ================
> Comment at: include/llvm/ProfileData/InstrProf.h:43
> @@ -39,1 +42,3 @@
>
> +const std::error_category &instrprof_category();
> +
> ----------------
> This line move seems unnecessary.
>
> ================
> Comment at: include/llvm/ProfileData/InstrProf.h:54
> @@ +53,3 @@
> +    reserved_2 = 2,
> +    reserved_3 = 3,
> +    last = 3,
> ----------------
> Any idea on why reserving 3 slots? Why not more? Or fewer? I guess I'm
> asking what other kinds of value profiling do you have in mind for future
> changes.
>

I missed this one. Three is good enough for the near future, for instance
stringOp value profiling, div/rem value profiling, etc.

David


> ================
> Comment at: include/llvm/ProfileData/InstrProfReader.h:18
> @@ -17,3 +17,2 @@
>
> -#include "llvm/ADT/ArrayRef.h"
>  #include "llvm/ADT/StringExtras.h"
> ----------------
> Please keep this. The file still uses ArrayRefs.
>
> ================
> Comment at: include/llvm/ProfileData/InstrProfReader.h:249
> @@ +248,3 @@
> +      case 2: return ReadDataV1V2(K, D, N);
> +      case 3: return ReadDataV3(K, D, N);
> +    }
> ----------------
> A question on format evolution. How far back should we go as the file
> format evolves?
>
> ================
> Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4923
> @@ -4923,1 +4922,3 @@
> +  case Intrinsic::instrprof_instrument_target:
> +    llvm_unreachable("instrprof failed to lower an value profiling call");
>    case Intrinsic::frameescape: {
> ----------------
> s/an value/a value/
>
> ================
> Comment at: lib/ProfileData/InstrProfReader.cpp:306
> @@ -283,2 +305,3 @@
> +    std::vector<uint64_t> Counts;
>      Counts.clear();
>      Counts.reserve(RawCounts.size());
> ----------------
> No need to clear Counts if you just declared it.
>
> ================
> Comment at: lib/ProfileData/InstrProfReader.cpp:399
> @@ +398,3 @@
> +      endian::readNext<uint64_t, little, unaligned>(D);
> +    if (1 != FormatVersion)
> +      ++I;
> ----------------
> I find FormatVersion != 1 easier to read.
>
> ================
> Comment at: test/Instrumentation/InstrProfiling/linkage.ll:26
> @@ -25,3 +25,3 @@
>  ; CHECK: @"__llvm_profile_counters_linkage.ll:foo_internal" = internal
> global
> -; CHECK: @"__llvm_profile_data_linkage.ll:foo_internal" = internal
> constant
> +; CHECK: @"__llvm_profile_data_linkage.ll:foo_internal" = internal
>  define internal void @foo_internal() {
> ----------------
> Any way that we could keep these variables in .rodata?
>
> http://reviews.llvm.org/D8908
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150613/88c1c8a3/attachment.html>