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

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

Hi Daniel,

Is this NFC in general, or just NFC with regards to the existing AArch64 description? More to the point, is it possible to concoct a useful test case that would fail without this ordering?

Also, will the priorities always be the same for all targets, or is it going to be possible to tweak things?

Thanks,
Diana



================
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.
+  ///
----------------
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).


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:171
+  ///
+  /// The only one that's known to matter at the moment is that OPM_Int must
+  /// have priority over OPM_RegBank. This is because gMIR represents constants
----------------
OPM_Int doesn't exist yet (you're introducing it in the next patch)


================
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
----------------
Just curious: have you given any thought to how this would work for vectors?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:231
+    //        done this way because we can't delve into LLT's yet.
+    if (Kind == B.getKind())
+      return Ty > static_cast<const LLTOperandMatcher &>(B).Ty;
----------------
Nitpick: I would replace this with

```
if (B.OperandPredicateMatcher::isHigherPriorityThan(*this))
  return false;
```
and then compare Ty's. I think that's more robust against changes to OPM::isHigherPriorityThan.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:299
+    if (predicates_size() > B.predicates_size())
+      return true;
+
----------------
Shouldn't you also return false if predicates_size() < B.predicates_size() ?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:417
+    // Instruction matchers involving more operands have higher priority.
+    if (Operands.size() > B.Operands.size())
+      return true;
----------------
Same as above, return false when it's the other way around.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:538
+    // Rules involving more match roots have higher priority.
+    if (Matchers.size() > B.Matchers.size())
+      return true;
----------------
Same as above.


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


https://reviews.llvm.org/D29711