[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
llvm-commits at lists.llvm.org
Mon Feb 13 01:38:58 PST 2017
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
More information about the llvm-commits
mailing list