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

Kristof Beyls via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 10 10:00:43 PST 2017


kristof.beyls 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:
> kristof.beyls wrote:
> > ab wrote:
> > > 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.
> > I'm trying to follow the discussion here, but find it a bit hard to understand in depth.
> > I'm wondering if priority of patterns/rules is written up somewhere already?
> > (This comment seems to suggest it's still very much an in-progress design discussion, so isn't written up anywhere?)
> > I'm assuming the priority of which pattern to try and match first will remain the same as under DAGISel? Or do we consider making some tweaks?
> > If it remains the same, what further ordering rules are being discussed here, and why does more ordering need to be defined compared to DAGISel?
> > 
> > Please don't let my comment slow you down in getting to a solution, but I think it'd be useful for this area to be explained a bit better in the end somewhere, e.g. in the comment at the top of this file.
> > Doesn't necessarily need to be as part of this patch.
> > 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.
> 
> I think a certain amount of ordering is unavoidable because we have a mixture of special cases that overlap with general cases. Consider these two patterns:
>   (set GPR32:$rd, (add GPR32:$rs1, GPR32:$rs2))
>   (set GPR32:$rd, (add (mul GPR32:$rs1, GPR32:$rs2), GPR32:$rs3))
> If we check them in the order I've written them then we can never match a multiply-accumulate but the other way around will always match the multiply accumulate when it's possible to do so. An alternative is to pick the best match at run time but that prevents early exits from the matcher.
> 
> Another example is:
>   (set GPR32:$rd, (add GPR32:$rs1, uimm16:$imm))
>   (set GPR32:$rd, (add GPR32:$rs1, uimm8:$imm))
> On a variable-length instruction set, there may be code-size and/or performance gains from preferring the uimm8 version.
> 
> > I'm trying to follow the discussion here, but find it a bit hard to understand in depth.
> > I'm wondering if priority of patterns/rules is written up somewhere already?
> > (This comment seems to suggest it's still very much an in-progress design discussion, so
> > isn't written up anywhere?)
> 
> That's right, we haven't decided what kind of ordering GlobalISel really needs yet. At the moment I'm trying to stick to the minimum needed for the matcher to work correctly. By the time we start to optimise the generated matcher I'm hoping to have a better idea of what the ordering issues are.
> 
> For SelectionDAG it's defined in PatternSortingPredicate from utils/TableGen/DAGISelEmitter.cpp.
> 
> > I'm assuming the priority of which pattern to try and match first will remain the same
> > as under DAGISel? Or do we consider making some tweaks?
> 
> Maybe. Ideally, the imported rules have the same overall behaviour as they do in SelectionDAG so that we produce the same or better code out of the box but I'm not sure that's possible due to the differences in representation in GlobalISel. For example, LLT can't distinguish floating point from integer like MVT can so some of SelectionDAG's ordering rules cannot be transferred to GlobalISel.
Brilliant explanation, thanks! Much appreciated!


https://reviews.llvm.org/D29711





More information about the llvm-commits mailing list