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

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar marked 16 inline comments as done.
aditya_nandakumar added a comment.

Will post the updated diff shortly.



================
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)
----------------
dsanders wrote:
> 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.
Thanks. That sounds much better than what I picked.


================
Comment at: CMakeLists.txt:176
+if(DAG_PRINT_USED_ISEL_PATTERNS)
+  set(LLVM_TBLGEN_DAG_FLAGS "-instrument-coverage")
+endif()
----------------
ab wrote:
> 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'.
Makes sense (I was initially hesitating to add to that function) . I'll go ahead and add this.


================
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) {
----------------
dsanders wrote:
> SelectionDAGISel is already abstract (because of Select()) so I think we should use make these pure virtual with '= 0'.
Currently Tablegen emits definitions for those two functions only when -instrument-coverage is passed in. This means if we make it pure virtual, then those two methods would be unimplemented by the <Target>DAGISels when -instrument-coverage is not passed in.


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:68
+    auto It =
+        std::find(VecPatternsCovered.begin(), VecPatternsCovered.end(), P);
+    if (It == VecPatternsCovered.end()) {
----------------
ab wrote:
> dsanders wrote:
> > 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.
> You're absolutely right, seems like this should just be a vector.
The problem I saw was that EmitMatcherList was called several times (since we need child and offset of failure code before emitting either of them, they buffer the output to a temporary string). I noticed w/o deduplication - the same patterns show up several times.
Not sure if lower_bound will work as ordering by '<' for strings seem weird?

There's no reason to use pair currently - I'll just create the concatenated strings directly and have a vector<string>


Repository:
  rL LLVM

https://reviews.llvm.org/D29678