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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:169
+  /// When all more important distinctions between rules are equal, the order of
+  /// these definitions is the order the matches will be attempted.
+  ///
----------------
rovka wrote:
> I think this comment could be fleshed out a bit. The way it is phrased now, it isn't clear what other distinctions between rules are more important (so maybe an example would help). It also sounds like a last resort, when in fact we can have more specific criteria for predicates of the same kind (e.g. in LLTOperandMatcher::isHigherPriorityThan).
I see your point here. I'll see if I can think of a better explanation


================
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.
----------------
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.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:227
+    // 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
----------------
rovka wrote:
> Just curious: have you given any thought to how this would work for vectors?
Not yet. I'm not sure they need to have a priority ordering at the moment.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:299
+    if (predicates_size() > B.predicates_size())
+      return true;
+
----------------
rovka wrote:
> Shouldn't you also return false if predicates_size() < B.predicates_size() ?
I think that makes sense but I don't think it has a functional effect on the matcher. I'll make this change (and the others like it).


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:724
+            [](const RuleMatcher &A, const RuleMatcher &B) {
+              return A.isHigherPriorityThan(B);
+            });
----------------
rovka wrote:
> I'd add an assert here on the asymmetry of isHigherPriorityThan.
I'm not sure what you mean here.


https://reviews.llvm.org/D29711