[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:
> > 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.


https://reviews.llvm.org/D28942