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

[Dinesh Dwivedi]

Hi Nick,

Actually I looked at it and the problem is m_not just gets one value to check.
So it can not check if 2 values are NOT of each other.

Regards
Dinesh Dwivedi


------- Original Message -------
Sender : Nick Lewycky<nlewycky at google.com>
Date : Jun 20, 2014 00:13 (GMT+05:30)
Title : Re: Re: [PATCH] Added instruction combine to transform few more negative values addition to subtraction

On 19 June 2014 02:56, Dinesh Dwivedi <dinesh.d at samsung.com> wrote:

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?



Sounds like m_Not is missing something. Could I interest you in teaching it to handle m_Not(ConstantInt) by looking for ~CI?



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
>
>
>


_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits