[PATCH] D116231: [InstCombine] (~a & ~b & c) | (~a & ~c & b) --> (b ^ c) & ~a

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

In D116231#3225213 <https://reviews.llvm.org/D116231#3225213>, @spatel wrote:

> In D116231#3224577 <https://reviews.llvm.org/D116231#3224577>, @foad wrote:
>
>> I don't really have an opinion on the patch, but I'm curious.
>>
>> Given (~a & ~b & c) | (~a & ~c & b), do we make any attempt to pull out the ~a like this: ~a & ((~b & c) | (~c & b)) ? If not, why not? Would that be a good thing to address? Then it's just a case of simplifying the two-variable (~b & c) | (~c & b) -> b ^ c which is easy.
>
> InstCombine does try that transform via InstCombinerImpl::SimplifyAssociativeOrCommutative() and/or InstCombinerImpl::SimplifyUsingDistributiveLaws(). And that works as expected - for example if we alter the 1st modified test in this patch to be like this:
>
>   define i32 @not_and_not_and_and_or(i32 %a, i32 %b, i32 %c) {
>     %nota = xor i32 %a, -1
>     %notb = xor i32 %b, -1
>     %and1 = and i32 %nota, %c
>     %and2 = and i32 %and1, %notb
>     %or1 = or i32 %a, %c
>     %not1 = xor i32 %or1, -1
>     %and3 = and i32 %not1, %b
>     %or3 = or i32 %and2, %and3
>     call void @use(i32 %nota)
>     call void @use(i32 %notb)
>     ret i32 %or3
>
> Then it reduces because we find the 'b' and '~b' values directly in the operands of the 'and' instructions. So this might be a question for "-reassociate" - can we get that pass to arrange the operands such that -instcombine can fold this (without breaking some other pattern)?

Ressociate does that and in fact a simpler case works. The problem is with a more complex case where other subexpression is used elsewhere. For example if (~a & b) is used. In that case reassociate does not do it because it is not beneficial, and it only becomes beneficial when we consider a bigger pattern.

In fact the original motivating case has all possible subexpression permutations in its 255 logical functions, which then lead to mutlti-use cases.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116231/new/

https://reviews.llvm.org/D116231