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

[Daniel Sanders via 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
>