[PATCH] D24419: [InstCombine] use commutative matchers for patterns with commutative operators

[Sanjay Patel via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D24419#558577, @sanjoy wrote:

> In https://reviews.llvm.org/D24419#550211, @spatel wrote:
>
> > I'm not sure how to canonicalize this without possibly conflicting
> >  with other canonicalization rules based on operand complexity. Ie, we
> >  order binop operands depending on whether they are themselves
> >  binop/unop/params/constants.
>
>
> Yeah, I can't come up with a scheme that will obviously not clash with
>  InstCombine's operand complexity order.
>
> An alternative may be to canonicalize "internally" -- that is first
>  split the expression into `(Op0 ITy0 Op1) ITy1 (Op2 ITy2 Op3)` (that
>  is, "explode" the contents of the operation into local variables) and
>  then `std::swap` the `Op0` / `Op1` or the `Op2` / `Op3` pairs in
>  whatever order that makes matching these easier.  That may involve too
>  much refactoring though.


Ah, yes - "local" canonicalization. Some of that does already exist in these functions, but of course it's not done consistently. IMO, using "m_c_" is cleaner. Eg, in some places we're maintaining the swap state (see "SwappedForXor" - not sure if that's actually necessary), so it makes it harder to read the code.

I've looked over this file again, and I'm estimating ~50 potential places where we should account for commuted patterns. So it's not *that* bad. :)

If there are no objections to using "m_c_" in this way, I'd like to move forward on this patch. I can add "FIXME" comments ahead of visitAnd/visitOr/visitXor, and then we can add tests and clean up the rest of the matchers in follow-ups.


https://reviews.llvm.org/D24419