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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


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


https://reviews.llvm.org/D28942