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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

It will LGTM with some nits fixed in addition to the ones Ahmed mentioned.



================
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)
----------------
ab wrote:
> 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?
I had a similar comment typed but the name you suggest is better.


================
Comment at: include/llvm/CodeGen/SelectionDAGISel.h:218-226
+  /// getPatternForIndex - Patterns selected by tablegen during ISEL
+  virtual StringRef getPatternForIndex(unsigned index) {
+    llvm_unreachable("Tblgen should generate the implementation of this!");
+  }
+
+  /// getIncludePathForIndex - get the td source location of pattern instantiation
+  virtual StringRef getIncludePathForIndex(unsigned index) {
----------------
SelectionDAGISel is already abstract (because of Select()) so I think we should use make these pure virtual with '= 0'.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:3489
+      index |= (MatcherTable[MatcherIndex++] << 8);
+      dbgs() << "MORPHTO: " << getPatternForIndex(index) << "\n";
+      dbgs() << "INCLUDED: " << getIncludePathForIndex(index) << "\n";
----------------
'Morph' has a particular meaning so I'm not sure we should use it for both OPC_EmitNode* and OPC_MorphNodeTo*. I'd suggest 'COVERED: ' or similar.


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:62
 
+  typedef std::pair<std::string, std::string> PatternPair;
+  std::vector<PatternPair> VecPatternsCovered;
----------------
ab wrote:
> If we never look into the components, maybe just concatenate the strings into one when you create them and avoid the std::pair?
If we do need to keep the std::pair<> then I think there ought to be a comment here indicating that the first string is the src pattern and the second is the dst patten.


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:68
+    auto It =
+        std::find(VecPatternsCovered.begin(), VecPatternsCovered.end(), P);
+    if (It == VecPatternsCovered.end()) {
----------------
ab wrote:
> 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?
Do we really need to de-dupe them? I wouldn't expect there to be duplicates since this indicates redundant patterns in the tablegen files and the MatcherTable entries for one of them will be unreachable.


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:142
+
+    assert(VecPatternsCovered.size() < 65536 &&
+           "Using only 16 bits to encode offset into Pattern Table");
----------------
isUInt<16>(VecPatternsCovered.size())


================
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";
----------------
ab wrote:
> Can you use the standard naming style?  (index -> Index;  here and elsewhere)
Could you also add the missing whitespace before the '{' and emit indentation to the tablegen-erated file?


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:152-153
+         << VecPatternsCovered[i].second << "\"";
+      if (i < e - 1)
+        OS << ",\n";
+    }
----------------
I'd make this unconditional since trailing commas are ok


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:165-166
+      OS << "\"" << VecIncludeStrings[i] << "\"";
+      if (i < e - 1)
+        OS << ",\n";
+    }
----------------
Similarly, I'd make this unconditional.


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:625
   case Matcher::MorphNodeTo: {
+    bool Coverage_emitted = false;
+    if (InstrumentCoverage) {
----------------
Personally I prefer underscored names but we should use CamelCase. Similarly below.


Repository:
  rL LLVM

https://reviews.llvm.org/D29678