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

[Diana Picus via Phabricator via llvm-commits]

rovka 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.
----------------
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!


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:299
+    if (predicates_size() > B.predicates_size())
+      return true;
+
----------------
dsanders wrote:
> 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).
It might have a functional effect on the sort, but I'll admit I haven't thought enough about it to be sure. Anyway, it looks like a simple way to make sure we're preserving asymmetry at least in a few extra cases.


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

I meant assert that if A.isHigherPriorityThan(B) then !B.isHigherPriorityThan(A). I think this is important because we're using a pretty complex function for sorting and it's a good idea to check that it's a strict weak ordering, so we don't have trouble with the sort. I don't know if it's worth worrying about transitivity, but asymmetry (which implies irreflexivity) is easy enough to verify.


https://reviews.llvm.org/D29711