[PATCH] D57463: Add a module pass for order file instrumentation

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

There is a copy of InstrProfData.inc in compiler-rt, and that one needs to be kept in sync with the one in compiler side.



================
Comment at: include/llvm/ProfileData/InstrProfData.inc:682
 #define INSTR_PROF_COVMAP_SECT_NAME INSTR_PROF_COVMAP_COFF
+/* Order file for Pika. */
+#define INSTR_PROF_ORDERFILE_SECT_NAME INSTR_PROF_ORDERFILE_COFF
----------------
should probably remove mentioning of Pika here.


================
Comment at: include/llvm/ProfileData/InstrProfData.inc:696
 #define INSTR_PROF_COVMAP_SECT_NAME INSTR_PROF_COVMAP_COMMON
+/* Order file for Pika. */
+#define INSTR_PROF_ORDERFILE_SECT_NAME INSTR_PROF_ORDERFILE_COMMON
----------------
Same here.


================
Comment at: include/llvm/ProfileData/InstrProfData.inc:748
 
+#ifndef INSTR_ORDER_FILE_INC
+// The maximal # of functions: 128*1024 (the buffer size will be 128*4 KB).
----------------
the #ifndef is not needed. The whole macro definition section is already preprocessing guarded.


================
Comment at: include/llvm/ProfileData/InstrProfData.inc:754
+#endif /* INSTR_ORDER_FILE_INC */
 #else
 #undef INSTR_PROF_DATA_DEFINED
----------------
The #else part is not needed either.


================
Comment at: lib/Transforms/Instrumentation/InstrOrderFile.cpp:39
+
+static cl::opt<std::string> ClWriteMapping(
+    "orderfile-write-mapping",
----------------
Why is it called 'write mapping'? Sounds a little confusing.


================
Comment at: lib/Transforms/Instrumentation/InstrOrderFile.cpp:42
+    cl::desc(
+        "Write mapping from (module id, function id) to the function symbol"),
+    cl::Hidden);
----------------
Since mapping file is already needed, should this option has a default  value?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57463