[PATCH] D15212: [PGO] Value profiling text format reader/writer support

[Vedant Kumar via llvm-commits]

vsk added a comment.

Sorry for the latency, comments inline --


================
Comment at: lib/ProfileData/InstrProfReader.cpp:107
@@ -106,3 +106,3 @@
   StringRef buffer = Buffer.getBufferStart();
-  return count == 0 || std::all_of(buffer.begin(), buffer.begin() + count,
-    [](char c) { return ::isprint(c) || ::isspace(c); });
+  return count == 0 ||
+         std::all_of(buffer.begin(), buffer.begin() + count,
----------------
I'd prefer to keep format changes for separate NFC commits.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:127
@@ +126,3 @@
+  Line++;
+  for (uint32_t VK = 0; VK < NumValueKinds; VK++) {
+    if (Line.is_at_end())
----------------
This looks correct but a bit repetitive. What do you think of using a macro like:

    #define AdvanceAndReadInt(Line, Dst) \
        if ((Line).is_at_end()) \
            return error(instrprof_error::truncated); \
        if (((Line)++)->getAsInteger(10, (Dst)))
            return error(instrprof_error::truncated); \

You could move the various "uint32_t" definitions up, or define them within the macro.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:211
@@ -148,1 +210,3 @@
 
+  if (std::error_code EC = readValueProfileData(Record))
+    return EC;
----------------
This could use a comment like "// Check if we have value profile data, and if so, read it.".

================
Comment at: test/tools/llvm-profdata/Inputs/vp-malform.proftext:5
@@ +4,3 @@
+# RUN: llvm-profdata show -ic-targets  -all-functions %t.profdata | FileCheck %s --check-prefix=IC
+
+foo
----------------
I don't think these input files should have RUN lines.

================
Comment at: test/tools/llvm-profdata/Inputs/vp-malform.proftext:43
@@ +42,3 @@
+2
+foo+100
+foo2:1000
----------------
It's not obvious that this is the error. Please add notes to your tests which make it clear what's being tested.

================
Comment at: test/tools/llvm-profdata/Inputs/vp-malform.proftext:48
@@ +47,3 @@
+
+#IC: Indirect Call Site Count: 3
+#IC-NEXT:    Indirect Target Results: 
----------------
Ditto for CHECK lines.

================
Comment at: test/tools/llvm-profdata/Inputs/vp-malform2.proftext:5
@@ +4,3 @@
+# RUN: llvm-profdata show -ic-targets  -all-functions %t.profdata | FileCheck %s --check-prefix=IC
+
+foo
----------------
Same comments from the last file.

Could you trim this file down a bit? It seems very similar to the last one.

================
Comment at: test/tools/llvm-profdata/Inputs/vp-truncate.proftext:5
@@ +4,3 @@
+# RUN: llvm-profdata show -ic-targets  -all-functions %t.profdata | FileCheck %s --check-prefix=IC
+
+foo
----------------
Same note about the RUN lines.


http://reviews.llvm.org/D15212