[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 added inline comments.


================
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) {
----------------
aditya_nandakumar wrote:
> 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.
Good point. Tablegen could emit the llvm_unreachable version when not given -instrument-coverage but there's no strong reason to to that over what you have here.


================
Comment at: utils/TableGen/DAGISelMatcherEmitter.cpp:68
+    auto It =
+        std::find(VecPatternsCovered.begin(), VecPatternsCovered.end(), P);
+    if (It == VecPatternsCovered.end()) {
----------------
aditya_nandakumar wrote:
> 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>
> 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.

Ah ok, I see where the dupes come from now. In that case I'd suggest having some kind of map/set to make the dupe check less expensive for large numbers of patterns. MapVector might be a good choice given that we need to refer to the strings by their index. Using something else would mean we have to maintain the vector and map/set separately.

> Not sure if lower_bound will work as ordering by '<' for strings seem weird?

The assembly parser uses lower_bound for the mnemonic but the catch is that the vector has to be sorted for it to work correctly.


https://reviews.llvm.org/D29678