[PATCH] D24419: [InstCombine] use commutative matchers for patterns with commutative operators
Sanjay Patel via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 12 07:28:16 PDT 2016
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
More information about the llvm-commits
mailing list