llvm dev - Apr 2021 - GlobalISel and commutative binops in Pat?

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

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

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
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

> 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).
> 
> 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
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>