[PATCH] D29678: [Tablegen/DAG Debug] - Instrumenting table gen DAGGenISelDAG to allow printing selected patterns(and their sources) during ISEL

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added a comment.

A few minor nitpicks, but this seems like a nice feature!



================
Comment at: CMakeLists.txt:174
 
+option(DAG_PRINT_USED_ISEL_PATTERNS "Debug: Prints tablegen patterns that were used for selecting" OFF)
+if(DAG_PRINT_USED_ISEL_PATTERNS)
----------------
I'd still prefix the name with 'LLVM_ENABLE_': it makes it easier to figure out where individual options come from in ccmake or CMakeCache.txt.

Maybe LLVM_ENABLE_DAGISEL_COV or something?


================
Comment at: CMakeLists.txt:176
+if(DAG_PRINT_USED_ISEL_PATTERNS)
+  set(LLVM_TBLGEN_DAG_FLAGS "-instrument-coverage")
+endif()
----------------
Maybe do this directly in TableGen.cmake, appending to LLVM_TABLEGEN_FLAGS?  That way we don't need to change AArch64/CMakeLists.txt.

In theory, we should only add it to -gen-dagisel invocation, but in practice, there's only one binary, so the flag will just be useless on the other invocations.
You can fix that too by only adding -instrument-coverage to the flags inside tablegen(), only if ${ARGN} contains '-gen-dag-isel'.


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:62
 
+  typedef std::pair<std::string, std::string> PatternPair;
+  std::vector<PatternPair> VecPatternsCovered;
----------------
If we never look into the components, maybe just concatenate the strings into one when you create them and avoid the std::pair?


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:68
+    auto It =
+        std::find(VecPatternsCovered.begin(), VecPatternsCovered.end(), P);
+    if (It == VecPatternsCovered.end()) {
----------------
While this isn't a huge deal (because it's a debug feature), the complexity of std::find makes me twitchy.

Maybe use a UniqueVector or std::lower_bound?


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:140
+public:
+  void PrintPatternMatchTable(raw_ostream &OS, StringRef Prefix) {
+
----------------
Print -> Emit

Also, can you declare it with the other Emit* functions above, and define the body elsewhere in the file?


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:146
+           "The sizes of Pattern and include vectors should be the same");
+    OS << "StringRef getPatternForIndex(unsigned index) override{\n";
+    OS << "static const char* " << Prefix << "_PATTERN_MATCH_TABLE[] = {\n";
----------------
Can you use the standard naming style?  (index -> Index;  here and elsewhere)


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:147
+    OS << "StringRef getPatternForIndex(unsigned index) override{\n";
+    OS << "static const char* " << Prefix << "_PATTERN_MATCH_TABLE[] = {\n";
+
----------------
"char*" -> "char *" (here and elsewhere)


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:161
+    OS << "\nStringRef getIncludePathForIndex(unsigned index) override{\n";
+    OS << "static const char* " << Prefix << "_INCLUDE_PATH_TABLE[] = {\n";
+
----------------
Since these tables are defined inside the functions, maybe remove the Prefix and give them a regular name?


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:703
     const CompleteMatchMatcher *CM = cast<CompleteMatchMatcher>(N);
+    bool Coverage_emitted = false;
+    if (InstrumentCoverage) {
----------------
NumCoverageBytes or something? Gets rid of the magic '* 3' later.


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:811
+      if (InstrumentCoverage)
+        OS << "    bool returnValue = " << P.getSelectFunc();
+      else
----------------
Give it a name?  Maybe "Succeeded" or something?


Repository:
  rL LLVM

https://reviews.llvm.org/D29678