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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:124
+template <class Predicate>
+class WithPredicates {
+private:
----------------
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!


https://reviews.llvm.org/D28942