[llvm-dev] GlobalISel and commutative binops in Pat?

[Björn Pettersson A via llvm-dev]

> -----Original Message-----
> From: Daniel Sanders <daniel_l_sanders at apple.com>
> Sent: den 22 april 2021 02:01
> To: Quentin Colombet <qcolombet at apple.com>
> Cc: Björn Pettersson A <bjorn.a.pettersson at ericsson.com>; llvm-dev <llvm-
> dev at lists.llvm.org>
> Subject: Re: [llvm-dev] GlobalISel and commutative binops in Pat?
> 
> 
> 
> > On Apr 21, 2021, at 09:48, Quentin Colombet <qcolombet at apple.com> wrote:
> >
> > + Daniel
> >
> > Hi Björn,
> >
> >> On Apr 21, 2021, at 3:08 AM, Björn Pettersson A via llvm-dev <llvm-
> dev at lists.llvm.org> wrote:
> >>
> >> Hi!
> >>
> >> I'm working on porting an OOT-target to use GlobalISel.
> >>
> >> One thing that I've noticed is that if I for example have a Pat like
> this
> >>
> >> def : Pat<(add i16:$reg, MyImmLeaf:$imm), (add16 i16:$src, imm:$imm)>;
> >>
> >> it only matches if the G_ADD has the immediate operand as the second
> operand.
> >>
> >>
> >> Here are some questions related to that:
> >>
> >> 1) I've been trying to use the generic pre/post legalize combiner, but I
> can't see that any canonicalization related to putting the immediate as
> second operand happens (I think it is something that the DAGCombiner would
> do).
> >> Is this something not-yet-implemented that we want to do?
> >>
> >> 2) Isn't the old SelectionDAG ISel automatically handling matching
> during selection for commutative SDNodes? So even without any prior
> canonicalization the matcher should handle this. I mean, in general a
> commutative operation could take two predicated non-leaf operands, so we
> can't solve all kinds combinations using canonicalization.
> >> Is this something not-yet-implemented in the GISel matcher that we want
> to do?
> >
> > IIRC, at one point Daniel and I talked about supporting this in the
> matcher tables. I think we end up postponing that because we didn’t have
> any use case for this and the IR coming in was canonical (plus it would
> have increase the compile time for no good reasons at that point).
> 
> I think it was mostly that it wasn't doing much harm in practice. IIRC
> CodeGenDAGPatterns duplicates patterns for commutative operations but then
> goes on to filter a few cases out that DAGISel traditionally handled
> itself. I remember I was trying to figure out how to change the filter for
> GlobalISel without breaking DAGISel but I don't think I found a good
> solution
> 
> > On the canonicalization part, we don’t do a whole lot because the
> incoming IR is already supposed to be canonical, thus any non canonical
> representation comes from GISel transformations themself and can be fixed.
> > By default we have only limited canonicalization, but we can add more
> with the combiners and IIRC the machine IR builder does some of it as well
> (and I believe you can plug a custom machine IR builder that does more).


My main concerns here are in the line of this:

- Being able to write MIR-tests for the GlobalISel steps is neat,
  but if relying on some sort of canonical input I need to be careful
  when writing such test cases to actually use the canonical form.
  And I really need to track if the canonical form changes, or else I end
  up with lots of irrelevant test cases verifying that my in practice
  "dead" ISel patterns works, while I'm actually lacking patterns that
  match the new canonical form.
  It is at least a litte bit scary if my most basic ISel patterns suddenly
  depend on how opt is canonicalizing IR.

- If I want to implement regressions tests that verify that I get the
  same codegen regardless of the input IR (or if I already have lots of
  such tests for my target), then I basically need to run opt before llc
  in such lit tests.
  I'm not sure if instcombine, or an O0 pipeline, is enough as a
  "canonicalizer" in such cases, or if the GlobalISel pattern matcher is
  fine-tuned for the canonical form produced by an O<n> pipeline?

- How do I figure out if my old ISel patterns will work correct for
  GlobalISel. They might have been written using the non-canonical form.
  I guess I either need to rely on benchmarks suites, or write lots of
  lit test cases based on "the current canonical form". So maybe the real
  questions is "How do I figure out the current canonical form for a
  certain baseline commit?".

Additional short-comings IMHO:

- The canonical form isn't really documented anywhere afaik. This makes
  all of the above extra complicated.

- There is no verifier that checks that the IR has canonical form
  before ISel.

- For simple things like binop:s, isn't it stupid that the IR allows
  the non-canonical form in the first place if passes should expect
  that the non-canonical form isn't used.


> >
> > Take all this with a grain of salt, it’s been a while since I looked into
> GISel actual implementation.
> >
> > Cheers,
> > -Quentin
> >
> >
> >
> >>
> >>
> >> Maybe I've simply missed something, or is doing something wrong as this
> currently fails for me. Adding all the commutative patterns as new
> definitions might be a lot of work for any target, so I suppose this should
> work out-of-the-box somehow.
> >>
> >> /Björn
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> llvm-dev at lists.llvm.org
> >