| Message ID | SN2PR0701MB1024B0A9DCDCA6C3F71885518E390@SN2PR0701MB1024.namprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Oct 20, 2015 at 8:46 AM, Hurugalawadi, Naveen <Naveen.Hurugalawadi@caviumnetworks.com> wrote: > Hi, > >>> +/* Fold X + (X / CST) * -CST to X % CST. */ >>> This one is still wrong > Removed. > >>> I don't understand the point of the FLOAT_TYPE_P check. > The check was there in fold-const. So, just had the same check. > >>> Will we also simplify (A & B) - (A & ~B) into B - (A ^ B) ? > Done. > >>> or maybe integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1)) > Done. > >>> :c on bit_ior? It should also allow you to merge the 2 CST versions into one. > Had it. But due to the error on "logical_inverted_value"; confused it with > this pattern and had duplicated it. > >>> I am not really in favor of the indentation change (or the comment). > Done. > >>> This patch is ok when bootstrapped / tested and with a proper changelog entry. > Regression tested on X86_64 with no extra failures. +(simplify + (minus (bit_and:s @0 INTEGER_CST@2) (bit_and:s @0 INTEGER_CST@1)) + (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1))) + (minus (bit_xor @0 @1) @1))) use if (wi::bit_and (@2, @1) == 0) and instead of the 2nd group +/* Fold (A & B) - (A & ~B) into B - (A ^ B). */ +(simplify + (minus (bit_and:s @0 @1) (bit_and:s @0 (bit_not @1))) + (minus @1 (bit_xor @0 @1))) +(simplify + (minus (bit_and:s @0 INTEGER_CST@1) (bit_and:s @0 INTEGER_CST@2)) + (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1))) + (minus @1 (bit_xor @0 @1)))) place a :c on the minus of the one not matching INTEGER_CSTs. +(simplify + (bit_ior:c (bit_and @0 INTEGER_CST@2) (bit_and (bit_not @0) INTEGER_CST@1)) + (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1))) + (bit_xor @0 @1))) see above, use wi::bit_and instead Otherwise looks ok to me. RIchard. > 2015-10-20 Richard Biener <rguenther@suse.de> > Naveen H.S <Naveen.Hurugalawadi@caviumnetworks.com> > > * fold-const.c (fold_binary_loc) : Move (-A) * (-B) -> A * B > to match.pd. > Move (a * (1 << b)) is (a << b) to match.pd. > Move convert (C1/X)*C2 into (C1*C2)/X to match.pd. > Move ~X & X, (X == 0) & X, and !X & X are zero to match.pd. > Move X & ~X , X & (X == 0), and X & !X are zero to match.pd. > > * match.pd (mult:c @0 (convert? (lshift integer_onep@1 @2))): > New simplifier. > (mult (rdiv:s REAL_CST@0 @1) REAL_CST@2): New simplifier. > (bit_and:c (convert? @0) (convert? (bit_not @0))): New simplifier. > (bit_ior (bit_and:s @0 (bit_not:s @1)) (bit_and:s (bit_not:s @0) @1)) > : New simplifier. > (mult:c (convert1? (negate @0)) (convert2? negate_expr_p@1)): > New simplifier. > (match (logical_inverted_value @0) (truth_not @0)) : New Predicate. > > > > 2015-10-20 Richard Biener <rguenther@suse.de> > Naveen H.S <Naveen.Hurugalawadi@caviumnetworks.com> > > * fold-const.c (fold_binary_loc) : Move Fold (A & ~B) - (A & B) > into (A ^ B) - B to match.pd > Move (X & ~Y) | (~X & Y) is X ^ Y to match.pd. > > * match.pd (minus (bit_and:s @0 (bit_not @1)) (bit_and:s @0 @1)): > New simplifier. > (minus (bit_and:s @0 INTEGER_CST@2) (bit_and:s @0 INTEGER_CST@1)): > New simplifier. > (minus (bit_and:s @0 @1) (bit_and:s @0 (bit_not @1))): New simplifier. > (minus (bit_and:s @0 INTEGER_CST@1) (bit_and:s @0 INTEGER_CST@2)): > New simplifier. > (bit_ior:c (bit_and:c @0 (bit_not @1)) (bit_and:c (bit_not @0) @1)): > New simplifier. > (bit_ior:c (bit_and @0 INTEGER_CST@2) (bit_and (bit_not @0) INTEGER_CST@1)): > New simplifier.
On Tue, 20 Oct 2015, Richard Biener wrote: > On Tue, Oct 20, 2015 at 8:46 AM, Hurugalawadi, Naveen > <Naveen.Hurugalawadi@caviumnetworks.com> wrote: >> Hi, >> >>>> +/* Fold X + (X / CST) * -CST to X % CST. */ >>>> This one is still wrong >> Removed. >> >>>> I don't understand the point of the FLOAT_TYPE_P check. >> The check was there in fold-const. So, just had the same check. >> >>>> Will we also simplify (A & B) - (A & ~B) into B - (A ^ B) ? >> Done. >> >>>> or maybe integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1)) >> Done. >> >>>> :c on bit_ior? It should also allow you to merge the 2 CST versions into one. >> Had it. But due to the error on "logical_inverted_value"; confused it with >> this pattern and had duplicated it. >> >>>> I am not really in favor of the indentation change (or the comment). >> Done. >> >>>> This patch is ok when bootstrapped / tested and with a proper changelog entry. >> Regression tested on X86_64 with no extra failures. > > +(simplify > + (minus (bit_and:s @0 INTEGER_CST@2) (bit_and:s @0 INTEGER_CST@1)) > + (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1))) > + (minus (bit_xor @0 @1) @1))) > > use if (wi::bit_and (@2, @1) == 0) I believe the original transformation required @1 and @2 to be bit_not of each other, so this might not work. He had a suitable test using wi, it is my fault he changed it (sorry for not being clear enough that my remark wasn't meant as a patch). The reason was so we could change INTEGER_CST to CONSTANT_CLASS_P and handle vectors. > and instead of the 2nd group > > +/* Fold (A & B) - (A & ~B) into B - (A ^ B). */ > +(simplify > + (minus (bit_and:s @0 @1) (bit_and:s @0 (bit_not @1))) > + (minus @1 (bit_xor @0 @1))) > +(simplify > + (minus (bit_and:s @0 INTEGER_CST@1) (bit_and:s @0 INTEGER_CST@2)) > + (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1))) > + (minus @1 (bit_xor @0 @1)))) > > place a :c on the minus of the one not matching INTEGER_CSTs. Will that really work?
On Wed, Oct 21, 2015 at 8:48 AM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Tue, 20 Oct 2015, Richard Biener wrote: > >> On Tue, Oct 20, 2015 at 8:46 AM, Hurugalawadi, Naveen >> <Naveen.Hurugalawadi@caviumnetworks.com> wrote: >>> >>> Hi, >>> >>>>> +/* Fold X + (X / CST) * -CST to X % CST. */ >>>>> This one is still wrong >>> >>> Removed. >>> >>>>> I don't understand the point of the FLOAT_TYPE_P check. >>> >>> The check was there in fold-const. So, just had the same check. >>> >>>>> Will we also simplify (A & B) - (A & ~B) into B - (A ^ B) ? >>> >>> Done. >>> >>>>> or maybe integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1)) >>> >>> Done. >>> >>>>> :c on bit_ior? It should also allow you to merge the 2 CST versions >>>>> into one. >>> >>> Had it. But due to the error on "logical_inverted_value"; confused it >>> with >>> this pattern and had duplicated it. >>> >>>>> I am not really in favor of the indentation change (or the comment). >>> >>> Done. >>> >>>>> This patch is ok when bootstrapped / tested and with a proper changelog >>>>> entry. >>> >>> Regression tested on X86_64 with no extra failures. >> >> >> +(simplify >> + (minus (bit_and:s @0 INTEGER_CST@2) (bit_and:s @0 INTEGER_CST@1)) >> + (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1))) >> + (minus (bit_xor @0 @1) @1))) >> >> use if (wi::bit_and (@2, @1) == 0) > > > I believe the original transformation required @1 and @2 to be bit_not > of each other, so this might not work. Oh, sorry - indeed. It fails for @2 == @1 == 0. So using bit_xor is required or simply wi::bit_not (@2) == @1. > He had a suitable test using wi, > it is my fault he changed it (sorry for not being clear enough that my > remark wasn't meant as a patch). The reason was so we could change > INTEGER_CST to CONSTANT_CLASS_P and handle vectors. I realized that but until that happens we should stick with the faster and simpler wi:: interface. The tree interface will allocate extra garbage for computing the BIT_XOR_EXPR even when no transform is done thus I believe we'd want a integer_bit_not_of_the_other () function avoiding extra tree garbage in the end. >> and instead of the 2nd group >> >> +/* Fold (A & B) - (A & ~B) into B - (A ^ B). */ >> +(simplify >> + (minus (bit_and:s @0 @1) (bit_and:s @0 (bit_not @1))) >> + (minus @1 (bit_xor @0 @1))) >> +(simplify >> + (minus (bit_and:s @0 INTEGER_CST@1) (bit_and:s @0 INTEGER_CST@2)) >> + (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1))) >> + (minus @1 (bit_xor @0 @1)))) >> >> place a :c on the minus of the one not matching INTEGER_CSTs. > > > Will that really work? Yes. The :c will create a 2nd pattern with the ops of the minus swapped, thus /* Fold (A & ~B) - (A & B) into (A ^ B) - B. */ (simplify (minus:c (bit_and:s @0 (bit_not @1)) (bit_and:s @0 @1)) (minus (bit_xor @0 @1) @1)) (simplify (minus (bit_and:s @0 INTEGER_CST@2) (bit_and:s @0 INTEGER_CST@1)) (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1))) (minus (bit_xor @0 @1) @1))) will implicitely add (simplify (minus (bit_and:s @0 @1) (bit_and:s @0 (bit_not @1)) (minus (bit_xor @0 @1) @1)) hmm, but that isn't correct for @1 == 0? So the fold-const.c code is incorrect. Note it doesn't trigger for int foo (int a, int b) { return (a & ~b) - (a & b); } because canonicalization makes it appear as (~b & a) - (a & b) and the fold-const.c fails to have a :c on the bit_and with the bit_not (you too). Note how the fold-const.c code doesn't care about where the ~ is placed (and for constants there isn't any explicit ~ anyway). The code triggers on int foo (int a, int b) { return ((a+1) & ~b) - ((a+1) & b); } int bar (int a, int b) { return ((a+1) & b) - ((a+1) & ~b); } and correctly it seems. The key is that it explicitely uses bit_not of the first ops b or ~b while the pattern tries to be clever and re-uses the present bit_not - _this_ doesn't play well with using :c. Still the duplicate INTEGER_CST pattern is not necessary. So I suggest to modify your patch to do +/* Fold (A & ~B) - (A & B) into (A ^ B) - B. */ +(simplify + (minus (bit_and:cs @0 (bit_not @1)) (bit_and:s @0 @1)) + (minus (bit_xor @0 @1) @1)) +(simplify + (minus (bit_and:s @0 INTEGER_CST@2) (bit_and:s @0 INTEGER_CST@1)) + (if (wi::bit_not (@2) == @1) + (minus (bit_xor @0 @1) @1))) + +/* Fold (A & B) - (A & ~B) into B - (A ^ B). */ +(simplify + (minus (bit_and:s @0 @1) (bit_and:cs @0 (bit_not @1))) + (minus @1 (bit_xor @0 @1))) Richard. > > -- > Marc Glisse
diff --git a/gcc/fold-const.c b/gcc/fold-const.c index de45a2c..3544949 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -9692,28 +9692,6 @@ fold_binary_loc (location_t loc, fold_convert_loc (loc, type, TREE_OPERAND (arg0, 0))); - if (! FLOAT_TYPE_P (type)) - { - /* Fold (A & ~B) - (A & B) into (A ^ B) - B, where B is - any power of 2 minus 1. */ - if (TREE_CODE (arg0) == BIT_AND_EXPR - && TREE_CODE (arg1) == BIT_AND_EXPR - && operand_equal_p (TREE_OPERAND (arg0, 0), - TREE_OPERAND (arg1, 0), 0)) - { - tree mask0 = TREE_OPERAND (arg0, 1); - tree mask1 = TREE_OPERAND (arg1, 1); - tree tem = fold_build1_loc (loc, BIT_NOT_EXPR, type, mask0); - - if (operand_equal_p (tem, mask1, 0)) - { - tem = fold_build2_loc (loc, BIT_XOR_EXPR, type, - TREE_OPERAND (arg0, 0), mask1); - return fold_build2_loc (loc, MINUS_EXPR, type, tem, mask1); - } - } - } - /* Fold __complex__ ( x, 0 ) - __complex__ ( 0, y ) to __complex__ ( x, -y ). This is not the same for SNaNs or if signed zeros are involved. */ @@ -10013,28 +9991,6 @@ fold_binary_loc (location_t loc, arg1); } - /* (X & ~Y) | (~X & Y) is X ^ Y */ - if (TREE_CODE (arg0) == BIT_AND_EXPR - && TREE_CODE (arg1) == BIT_AND_EXPR) - { - tree a0, a1, l0, l1, n0, n1; - - a0 = fold_convert_loc (loc, type, TREE_OPERAND (arg1, 0)); - a1 = fold_convert_loc (loc, type, TREE_OPERAND (arg1, 1)); - - l0 = fold_convert_loc (loc, type, TREE_OPERAND (arg0, 0)); - l1 = fold_convert_loc (loc, type, TREE_OPERAND (arg0, 1)); - - n0 = fold_build1_loc (loc, BIT_NOT_EXPR, type, l0); - n1 = fold_build1_loc (loc, BIT_NOT_EXPR, type, l1); - - if ((operand_equal_p (n0, a0, 0) - && operand_equal_p (n1, a1, 0)) - || (operand_equal_p (n0, a1, 0) - && operand_equal_p (n1, a0, 0))) - return fold_build2_loc (loc, BIT_XOR_EXPR, type, l0, n1); - } - /* See if this can be simplified into a rotate first. If that is unsuccessful continue in the association code. */ goto bit_rotate; diff --git a/gcc/match.pd b/gcc/match.pd index f3813d8..4bb1cc2 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -324,6 +324,33 @@ along with GCC; see the file COPYING3. If not see (if (real_isinteger (&TREE_REAL_CST (@1), &n) && (n & 1) == 0) (pows @0 @1)))))) +/* Fold (A & ~B) - (A & B) into (A ^ B) - B. */ +(simplify + (minus (bit_and:s @0 (bit_not @1)) (bit_and:s @0 @1)) + (minus (bit_xor @0 @1) @1)) +(simplify + (minus (bit_and:s @0 INTEGER_CST@2) (bit_and:s @0 INTEGER_CST@1)) + (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1))) + (minus (bit_xor @0 @1) @1))) + +/* Fold (A & B) - (A & ~B) into B - (A ^ B). */ +(simplify + (minus (bit_and:s @0 @1) (bit_and:s @0 (bit_not @1))) + (minus @1 (bit_xor @0 @1))) +(simplify + (minus (bit_and:s @0 INTEGER_CST@1) (bit_and:s @0 INTEGER_CST@2)) + (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1))) + (minus @1 (bit_xor @0 @1)))) + +/* Simplify (X & ~Y) | (~X & Y) -> X ^ Y. */ +(simplify + (bit_ior:c (bit_and:c @0 (bit_not @1)) (bit_and:c (bit_not @0) @1)) + (bit_xor @0 @1)) +(simplify + (bit_ior:c (bit_and @0 INTEGER_CST@2) (bit_and (bit_not @0) INTEGER_CST@1)) + (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1))) + (bit_xor @0 @1))) + /* X % Y is smaller than Y. */ (for cmp (lt ge) (simplify @@ -543,7 +570,7 @@ along with GCC; see the file COPYING3. If not see (match negate_expr_p VECTOR_CST (if (FLOAT_TYPE_P (TREE_TYPE (type)) || TYPE_OVERFLOW_WRAPS (type)))) - + /* -(A + B) -> (-B) - A. */ (simplify (negate (plus:c @0 negate_expr_p@1))