[PATCH] D29711: [globalisel] Sort RuleMatchers by priority.

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added inline comments.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:224-230
+    // Wider types should have priority over narrower types within the same
+    // family but the order of the families doesn't matter. So s64 is more
+    // important than s32 and f64 is more important than f32 but s64 and f64 are
+    // unordered.
+    // FIXME: Don't do a lexical comparison of the type name. Check the size
+    //        instead. This only works for the cases we emit so far and is only
+    //        done this way because we can't delve into LLT's yet.
----------------
rovka wrote:
> dsanders wrote:
> > ab wrote:
> > > dsanders wrote:
> > > > ab wrote:
> > > > > Huh, how can this be a problem?
> > > > It works fine for s32 and s64 since "s32" < "s64" but the overall ordering is wrong when the number of digits vary:
> > > >   "s128" < "s16" < "s32" < "s64" < "s8"
> > > > Once I've dealt with the LLT layering issue we'll be able to do a numeric comparison for the size such that the order is:
> > > >   s8 < s16 < s32 < s64 < s128
> > > Sorry; I'm asking why does the ordering matter in the first place?
> > Ah ok. Thinking about it again, I'm not sure it does. My original thinking was partly based on a comment from D26878 that talked about the order being correct by luck but we don't have the widenable concept at the moment. The other part of the thinking was based on the order I've seen the matcher tables be emitted but I think that order stems more from additional Predicates<> than from the type.
> Ha, I had the same comment in mind!
Ah yeah you're right, if we want to reinstate the isWidenable logic, we'd need this ordering.  But one of the reasons why I'm more and more convinced it's a bad idea is precisely the need for an ordering:  I have a gut feeling the patterns should never require that for correctness, letting us order them as we please for performance.  In which case, the types should never be a distinguishing factor (as we're only looking at performance indicators for breaking "ties" between "equivalent" patterns).  Does that sound right?  If so, I guess the type should just be ignored entirely.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:302-303
+    // This assumes that predicates are added in a consistent order.
+    for (const auto &Predicate : zip(predicates(), B.predicates())) {
+      if (std::get<0>(Predicate)->isHigherPriorityThan(*std::get<1>(Predicate)))
+        return true;
----------------
Ooh, nice!  It's unfortunate that we still have to deal with std::get<> =(


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:536
+  /// Returns true if this object is more important than B.
+  bool isHigherPriorityThan(const RuleMatcher &B) const {
+    // Rules involving more match roots have higher priority.
----------------
This should maybe take into account the AddedComplexity somehow, but that's not really compatible with the isHigherPriorityThan scheme :/

I'm not sure using AddedComplexity is a good idea in the first place: it's very magic-number-y and totally SDAG implementation specific.  Still, care has been put into that, so if there's any value we can easily extract from that, let's do it.

Maybe instead sum up the "complexity" of each Matcher?  We'd want to make sure to prioritize significant metrics like more instructions over operand kinds and such.

Assuming you don't have another usage in mind for the *Kind enums, that might let you simplify that to virtual methods in each Matcher returning its cost.


https://reviews.llvm.org/D29711