[PATCH] D28942: [globalisel] Re-factor ISel matchers into a hierarchy. NFC

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added inline comments.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:124
+template <class Predicate>
+class WithPredicates {
+private:
----------------
dsanders wrote:
> kristof.beyls wrote:
> > dsanders wrote:
> > > kristof.beyls wrote:
> > > > dsanders wrote:
> > > > > ab wrote:
> > > > > > The name is cryptic ...but I don't have a better idea  ¯\_(ツ)_/¯
> > > > > > 
> > > > > > PredicateList? PredicateVector? PredicateHolder?
> > > > > I struggled with ideas for the name too. The only reason I went with 'WithPredicates' is because it made the subclass declaration read nicely:
> > > > > > class OperandMatcher : public WithPredicates<OperandPredicateMatcher>
> > > > > which defines an matcher with predicates.
> > > > > 
> > > > > I'll have another think on naming.
> > > > I'm not entirely sure, but would PredicateMatcher be more in line with the terminology of the other classes used here?
> > > > And maybe the class hierarchy becomes a bit more simple to understand if the inheritance went roughly like follows:
> > > > 
> > > > ```
> > > > class PredicateMatcher; /* rename of current WithPredicates. */
> > > > class OperandPredicateMatcher : PredicateMatcher<OperandPredicateMatcher>;
> > > > class InstructionPredicateMatcher : PredicateMatcher<InstructionPredicateMatcher>;
> > > > class OperandMatcher;
> > > > class InstructionMatcher;
> > > > ```
> > > > rather than the current
> > > > 
> > > > ```
> > > > class WithPredicates; /* rename of current WithPredicates. */
> > > > class OperandPredicateMatcher : PredicateMatcher<OperandPredicateMatcher>;
> > > > class InstructionPredicateMatcher : PredicateMatcher<InstructionPredicateMatcher>;
> > > > class OperandMatcher : WithPredicates<OperandPredicateMatcher>;
> > > > class InstructionMatcher : WithPredicates<InstructionPredicateMatcher>;
> > > > ```
> > > > I'm afraid I haven't had much thought about this, but the first seems a bit more intuitive/easier to understand to me, assuming it's possible at all to define the class hierarchy like that.
> > > > 
> > > > Anyway, as I think we're still in the exploring phase for how the tablegen-generator will work, I don't think it's worthwhile to obsess too much on the last polishing touches of the OO-design of this part of the tablegen-generator right now, if it would be delaying further exploration of the tablegen-generator.
> > > > I'm not entirely sure, but would PredicateMatcher be more in line with the terminology of the other classes used here?
> > > 
> > > It would need to be PredicatesMatcher since it provides functions to add and emit code for multiple predicates.
> > > 
> > > > And maybe the class hierarchy becomes a bit more simple to understand if the inheritance went roughly like follows:
> > > >   class PredicateMatcher; /* rename of current WithPredicates. */
> > > >   class OperandPredicateMatcher : PredicateMatcher<OperandPredicateMatcher>;
> > > >   class InstructionPredicateMatcher : PredicateMatcher<InstructionPredicateMatcher>;
> > > >   class OperandMatcher;
> > > >   class InstructionMatcher;
> > > 
> > > In this hierarchy, each predicate would contain a list of predicates.
> > Ah yes. PredicatesMatcher as a name is more clear to me than "WithPredicates". The hierarchy you have in your most recent patch also seems to make sense to me. Thanks for explaining!
> Thinking about it a bit more this morning. It's easy to mis-read PredicatesMatcher as PredicateMatcher which then makes the hierarchy look wrong. I'll go with PredicateListMatcher to make that a bit clearer.
SGTM;  maybe rename emitCxxPredicatesExpr to emitCxxPredicateListExpr, for the same reason?

BTW, this naming scheme has an interesting quirk: how should we call top-level Predicates (non-instruction, as in, the actual Predicate class in Target.td).  Locally, I went with the terrible but consistent PredicatePredicateMatcher.  Any ideas?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:163
+  template <class Arg1>
+  void emitCxxPredicatesExpr(raw_ostream &OS, Arg1&& arg1) const {
+    if (Predicates.empty()) {
----------------
I attempted to merge these into one in r293214;  was that what you had in mind?  Looks like std::forward<Args...>  should have been std::forward<Args> ?


https://reviews.llvm.org/D28942