[PATCH] Added instruction combine to transform few more negative values addition to subtraction

[Dinesh Dwivedi]

Hi Nick,

m_Not checks for pattern XOR(X, -1) and will not work in current scenario.
i.e. it can check if we have XOR(X, -1) pattern but can not identify that
0x55555555 and ~(0xAAAAAAAA) are NOT of each other.

Am I missing something?

Regards
Dinesh Dwivedi

------- Original Message -------
Sender : Nick Lewycky<nicholas at mxc.ca> 
Date   : Jun 19, 2014 11:50 (GMT+05:30)
Title  : Re: [PATCH] Added instruction combine to transform few more negative
 values addition to subtraction

Nick Lewycky wrote:
> Sadness. Did you know that gcc gets this even at -O0? Not this pattern mind you, but the original testcase. It's that simple, at least before we transform to xor(or) form. GCC doesn't get it in xor(or) form either.
>
> I looked for a better way to address this optimizer deficiency and didn't find one. The one thing I didn't check is whether we could get it by reordering our optimizations inside instcombine and do something else before it gets into xor(or) form. Even if that did fix the testcase in PR14365 it would still leave the case where someone actually does write "((a|~c) ^ c) + (a+1)".

Actually, I was just thinking about this some more. There's no reason 
'C' needs to be a constant at all, is there? As long as it's X and ~X 
(use m_Value(X) then m_Not(m_Specific(X))) the transform should be 
correct, right?

Nick

>
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:928
> @@ +927,3 @@
> +
> +  // This function creates 2 instructions to replace ADD, we need atleast one of
> +  // LHS or RHS to have one use to ensure benefit in transform.
> ----------------
> "atleast" -->  "at least"
>
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:951
> @@ +950,3 @@
> +    if (match(X, m_Xor(m_Value(Y), m_APInt(C1)))) {
> +      if (match(Y, m_Or(m_Value(Z), m_APInt(C2)))&&  (*C2 == ~(*C1))) {
> +        Value *NewAnd = Builder->CreateAnd(Z, *C1);
> ----------------
> Optional: *C2 == ~(*C1) might be more efficient as !C2->intersects(*C1). It isn't, but in theory we could improve intersects to not compute an intermediate value (ie., it doesn't need to look at all the bits once it finds a pair are set in both).
>
> ================
> Comment at: test/Transforms/InstCombine/add2.ll:142
> @@ +141,1 @@
> +}
>  No newline at end of file
>
> ----------------
> Please make sure there's a newline at the end of the file.
>
> http://reviews.llvm.org/D3733
>
>
>