| Message ID | 51C12EC5.709@redhat.com |
|---|---|
| State | New |
| Headers | show |
2013/6/19 Jeff Law <law@redhat.com>: > > * gcc.dg/tree-ssa/forwprop-28.c: New test. > In the gnu coding standard we have a space before the open-parentheses. Would that be great to have testcase follow this convention as well? :) If so, then... > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/forwprop-28.c > b/gcc/testsuite/gcc.dg/tree-ssa/forwprop-28.c > new file mode 100644 > index 0000000..2c42065 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/forwprop-28.c > @@ -0,0 +1,76 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-tree-forwprop1" } */ > + > +extern char * frob (void); > +extern _Bool testit(void); Missing a space before '('. > + > +test (int code) > +{ > + char * temp = frob();; Likewise. And redundant ';'. > + int rotate = (code == 22); > + if (temp == 0 && !rotate) > + oof(); Likewise. > +} > + > +test_2 (int code) > +{ > + char * temp = frob(); Likewise. > + int rotate = (code == 22); > + if (!rotate && temp == 0) > + oof(); Likewise. And there are similar cases can be fixed in test_3, test_4, test_5, test_6, test_7, and test_8. Best regards, jasonwucj
On Wed, Jun 19, 2013 at 03:02:38PM +0800, Chung-Ju Wu wrote: > In the gnu coding standard we have a space before > the open-parentheses. Would that be great to have > testcase follow this convention as well? :) > > If so, then... Testcases generally don't need to follow the coding conventions, they can and often it is nicer if they follow it, but we certainly also want testcases that don't follow it, otherwise we wouldn't have testsuite coverage for other coding styles (what if say a parser or preprocessor didn't work properly if there wasn't a space in between function name and ( ?). Jakub
2013/6/19 Jakub Jelinek <jakub@redhat.com>: > On Wed, Jun 19, 2013 at 03:02:38PM +0800, Chung-Ju Wu wrote: >> In the gnu coding standard we have a space before >> the open-parentheses. Would that be great to have >> testcase follow this convention as well? :) >> >> If so, then... > > Testcases generally don't need to follow the coding conventions, > they can and often it is nicer if they follow it, but we certainly > also want testcases that don't follow it, otherwise we wouldn't have > testsuite coverage for other coding styles (what if say a parser or > preprocessor didn't work properly if there wasn't a space in between > function name and ( ?). > > Jakub That makes sense. Thanks for clarifying it. :) Best regards, jasonwucj
On Wed, Jun 19, 2013 at 6:08 AM, Jeff Law <law@redhat.com> wrote: > > The notable changes since the last version: > > First, it should properly handle signed single bit types, though I haven't > tested it with real code. > > Second, the transformation is only applied when the result is used in a > conditional. Thus it's much less likely to pessimize targets with and-not > instructions as it's highly likely we'll eliminate two gimple statements > rather than just one. > > > Other comments (such as not needing to retrieve gsi_stmt) were also > addressed. Testcase was renamed, but is otherwise unchanged. > > Bootstrapped and regression tested on x86_64-unknown-linux-gnu. > > OK for the trunk? Ok. Thanks, Richard. > > * tree-ssa-forwprop.c (simplify_bitwise_binary_boolean): New > function. > (simplify_bitwise_binary): Use it to simpify certain binary ops on > booleans. > > * gcc.dg/tree-ssa/forwprop-28.c: New test. > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/forwprop-28.c > b/gcc/testsuite/gcc.dg/tree-ssa/forwprop-28.c > new file mode 100644 > index 0000000..2c42065 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/forwprop-28.c > @@ -0,0 +1,76 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-tree-forwprop1" } */ > + > +extern char * frob (void); > +extern _Bool testit(void); > + > +test (int code) > +{ > + char * temp = frob();; > + int rotate = (code == 22); > + if (temp == 0 && !rotate) > + oof(); > +} > + > +test_2 (int code) > +{ > + char * temp = frob(); > + int rotate = (code == 22); > + if (!rotate && temp == 0) > + oof(); > +} > + > + > +test_3 (int code) > +{ > + char * temp = frob(); > + int rotate = (code == 22); > + if (!rotate || temp == 0) > + oof(); > +} > + > + > +test_4 (int code) > +{ > + char * temp = frob(); > + int rotate = (code == 22); > + if (temp == 0 || !rotate) > + oof(); > +} > + > + > +test_5 (int code) > +{ > + _Bool temp = testit();; > + _Bool rotate = (code == 22); > + if (temp == 0 && !rotate) > + oof(); > +} > + > +test_6 (int code) > +{ > + _Bool temp = testit(); > + _Bool rotate = (code == 22); > + if (!rotate && temp == 0) > + oof(); > +} > + > + > +test_7 (int code) > +{ > + _Bool temp = testit(); > + _Bool rotate = (code == 22); > + if (!rotate || temp == 0) > + oof(); > +} > + > + > +test_8 (int code) > +{ > + _Bool temp = testit(); > + _Bool rotate = (code == 22); > + if (temp == 0 || !rotate) > + oof(); > +} > + > +/* { dg-final { scan-tree-dump-times "Replaced" 8 "forwprop1"} } */ > diff --git a/gcc/tree-ssa-forwprop.c b/gcc/tree-ssa-forwprop.c > index c6a7eaf..29a0bb7 100644 > --- a/gcc/tree-ssa-forwprop.c > +++ b/gcc/tree-ssa-forwprop.c > @@ -1870,6 +1870,52 @@ hoist_conversion_for_bitop_p (tree to, tree from) > return false; > } > > +/* GSI points to a statement of the form > + > + result = OP0 CODE OP1 > + > + Where OP0 and OP1 are single bit SSA_NAMEs and CODE is either > + BIT_AND_EXPR or BIT_IOR_EXPR. > + > + If OP0 is fed by a bitwise negation of another single bit SSA_NAME, > + then we can simplify the two statements into a single LT_EXPR or LE_EXPR > + when code is BIT_AND_EXPR and BIT_IOR_EXPR respectively. > + > + If a simplification is mode, return TRUE, else return FALSE. */ > +static bool > +simplify_bitwise_binary_boolean (gimple_stmt_iterator *gsi, > + enum tree_code code, > + tree op0, tree op1) > +{ > + gimple op0_def_stmt = SSA_NAME_DEF_STMT (op0); > + > + if (!is_gimple_assign (op0_def_stmt) > + || (gimple_assign_rhs_code (op0_def_stmt) != BIT_NOT_EXPR)) > + return false; > + > + tree x = gimple_assign_rhs1 (op0_def_stmt); > + if (TREE_CODE (x) == SSA_NAME > + && INTEGRAL_TYPE_P (TREE_TYPE (x)) > + && TYPE_PRECISION (TREE_TYPE (x)) == 1 > + && TYPE_UNSIGNED (TREE_TYPE (x)) == TYPE_UNSIGNED (TREE_TYPE (op1))) > + { > + enum tree_code newcode; > + > + gimple stmt = gsi_stmt (*gsi); > + gimple_assign_set_rhs1 (stmt, x); > + gimple_assign_set_rhs2 (stmt, op1); > + if (code == BIT_AND_EXPR) > + newcode = TYPE_UNSIGNED (TREE_TYPE (x)) ? LT_EXPR : GT_EXPR; > + else > + newcode = TYPE_UNSIGNED (TREE_TYPE (x)) ? LE_EXPR : GE_EXPR; > + gimple_assign_set_rhs_code (stmt, newcode); > + update_stmt (stmt); > + return true; > + } > + return false; > + > +} > + > /* Simplify bitwise binary operations. > Return true if a transformation applied, otherwise return false. */ > > @@ -2117,8 +2163,44 @@ simplify_bitwise_binary (gimple_stmt_iterator *gsi) > return true; > } > } > - } > > + /* If arg1 and arg2 are booleans (or any single bit type) > + then try to simplify: > + > + (~X & Y) -> X < Y > + (X & ~Y) -> Y < X > + (~X | Y) -> X <= Y > + (X | ~Y) -> Y <= X > + > + But only do this if our result feeds into a comparison as > + this transformation is not always a win, particularly on > + targets with and-not instructions. */ > + if (TREE_CODE (arg1) == SSA_NAME > + && TREE_CODE (arg2) == SSA_NAME > + && INTEGRAL_TYPE_P (TREE_TYPE (arg1)) > + && TYPE_PRECISION (TREE_TYPE (arg1)) == 1 > + && TYPE_PRECISION (TREE_TYPE (arg2)) == 1 > + && (TYPE_UNSIGNED (TREE_TYPE (arg1)) > + == TYPE_UNSIGNED (TREE_TYPE (arg2)))) > + { > + use_operand_p use_p; > + gimple use_stmt; > + > + if (single_imm_use (gimple_assign_lhs (stmt), &use_p, &use_stmt)) > + { > + if (gimple_code (use_stmt) == GIMPLE_COND > + && gimple_cond_lhs (use_stmt) == gimple_assign_lhs (stmt) > + && integer_zerop (gimple_cond_rhs (use_stmt)) > + && gimple_cond_code (use_stmt) == NE_EXPR) > + { > + if (simplify_bitwise_binary_boolean (gsi, code, arg1, > arg2)) > + return true; > + if (simplify_bitwise_binary_boolean (gsi, code, arg2, > arg1)) > + return true; > + } > + } > + } > + } > return false; > } > >
On 06/19/2013 01:02 AM, Chung-Ju Wu wrote: > 2013/6/19 Jeff Law <law@redhat.com>: >> >> * gcc.dg/tree-ssa/forwprop-28.c: New test. >> > > In the gnu coding standard we have a space before > the open-parentheses. Would that be great to have > testcase follow this convention as well? :) > > If so, then... No reason not to fix the test in this instance. I'll make these updates before committing. jeff
On 19 June 2013 15:57, Jeff Law <law@redhat.com> wrote: > On 06/19/2013 01:02 AM, Chung-Ju Wu wrote: >> >> 2013/6/19 Jeff Law <law@redhat.com>: >>> >>> >>> * gcc.dg/tree-ssa/forwprop-28.c: New test. >>> >> >> In the gnu coding standard we have a space before >> the open-parentheses. Would that be great to have >> testcase follow this convention as well? :) >> >> If so, then... > > No reason not to fix the test in this instance. I'll make these updates > before committing. eh, nitpicking party ? + If a simplification is mode, return TRUE, else return FALSE. */ +static bool +simplify_bitwise_binary_boolean (gimple_stmt_iterator *gsi, s/mode/made/ Sounds nice, otherwise! thanks,
Jeff Law <law@redhat.com> writes:
> +/* { dg-final { scan-tree-dump-times "Replaced" 8 "forwprop1"} } */
$ grep -c Replaced forwprop-28.c.022t.forwprop1
16
;; Function test (test, funcdef_no=0, decl_uid=1388, symbol_order=0)
Replaced 'rotate_7 == 0' with '_6 == 0'
Replaced '_6 == 0' with 'code_5(D) != 22'
Andreas.
On 06/20/2013 04:49 AM, Andreas Schwab wrote: > Jeff Law <law@redhat.com> writes: > >> +/* { dg-final { scan-tree-dump-times "Replaced" 8 "forwprop1"} } */ > > $ grep -c Replaced forwprop-28.c.022t.forwprop1 > 16 > > ;; Function test (test, funcdef_no=0, decl_uid=1388, symbol_order=0) > > Replaced 'rotate_7 == 0' with '_6 == 0' > Replaced '_6 == 0' with 'code_5(D) != 22' Target? Give me enough information and I'll gladly take a look. Give me junk and I'll ignore. jeff
On Thu, Jun 20, 2013 at 2:52 PM, Jeff Law <law@redhat.com> wrote: > On 06/20/2013 04:49 AM, Andreas Schwab wrote: >> >> Jeff Law <law@redhat.com> writes: >> >>> +/* { dg-final { scan-tree-dump-times "Replaced" 8 "forwprop1"} } */ >> >> >> $ grep -c Replaced forwprop-28.c.022t.forwprop1 >> 16 >> >> ;; Function test (test, funcdef_no=0, decl_uid=1388, symbol_order=0) >> >> Replaced 'rotate_7 == 0' with '_6 == 0' >> Replaced '_6 == 0' with 'code_5(D) != 22' > > Target? > > Give me enough information and I'll gladly take a look. Give me junk and > I'll ignore. I see this now as well with match-and-simplify and a pending merge piece (soon to be committed, with testcase adjustment). We have a conflicting transform, so if you for example consider void test_4 (int code) { char *temp = frob (); int rotate = (code == 22); if (temp == 0 || !rotate) oof (); } then the conflicting transform is to simplify !(code == 22) to code != 22. If that happens first then the if () no longer is of X || !Y form but we see X || Y and thus your transform no longer applies (the transform meanwhile moved to match.pd - I am now moving !(code == 22) to code != 22 as well). Btw, I see better generated code on x86 when doing !(code == 22) -> code != 22 rather than transforming temp == 0 || !rotate to rotate <= (temp == 0). With your transform I see movl %edi, %ebx call frob cmpl $22, %ebx sete %dl testq %rax, %rax sete %al cmpb %al, %dl jbe .L17 while with the code != 22 one I see movl %edi, %ebx call frob testq %rax, %rax je .L28 cmpl $22, %ebx jne .L28 So I wonder if it is beneficial at all ... Thanks, Richard. > jeff
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/forwprop-28.c b/gcc/testsuite/gcc.dg/tree-ssa/forwprop-28.c new file mode 100644 index 0000000..2c42065 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/forwprop-28.c @@ -0,0 +1,76 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-forwprop1" } */ + +extern char * frob (void); +extern _Bool testit(void); + +test (int code) +{ + char * temp = frob();; + int rotate = (code == 22); + if (temp == 0 && !rotate) + oof(); +} + +test_2 (int code) +{ + char * temp = frob(); + int rotate = (code == 22); + if (!rotate && temp == 0) + oof(); +} + + +test_3 (int code) +{ + char * temp = frob(); + int rotate = (code == 22); + if (!rotate || temp == 0) + oof(); +} + + +test_4 (int code) +{ + char * temp = frob(); + int rotate = (code == 22); + if (temp == 0 || !rotate) + oof(); +} + + +test_5 (int code) +{ + _Bool temp = testit();; + _Bool rotate = (code == 22); + if (temp == 0 && !rotate) + oof(); +} + +test_6 (int code) +{ + _Bool temp = testit(); + _Bool rotate = (code == 22); + if (!rotate && temp == 0) + oof(); +} + + +test_7 (int code) +{ + _Bool temp = testit(); + _Bool rotate = (code == 22); + if (!rotate || temp == 0) + oof(); +} + + +test_8 (int code) +{ + _Bool temp = testit(); + _Bool rotate = (code == 22); + if (temp == 0 || !rotate) + oof(); +} + +/* { dg-final { scan-tree-dump-times "Replaced" 8 "forwprop1"} } */ diff --git a/gcc/tree-ssa-forwprop.c b/gcc/tree-ssa-forwprop.c index c6a7eaf..29a0bb7 100644 --- a/gcc/tree-ssa-forwprop.c +++ b/gcc/tree-ssa-forwprop.c @@ -1870,6 +1870,52 @@ hoist_conversion_for_bitop_p (tree to, tree from) return false; } +/* GSI points to a statement of the form + + result = OP0 CODE OP1 + + Where OP0 and OP1 are single bit SSA_NAMEs and CODE is either + BIT_AND_EXPR or BIT_IOR_EXPR. + + If OP0 is fed by a bitwise negation of another single bit SSA_NAME, + then we can simplify the two statements into a single LT_EXPR or LE_EXPR + when code is BIT_AND_EXPR and BIT_IOR_EXPR respectively. + + If a simplification is mode, return TRUE, else return FALSE. */ +static bool +simplify_bitwise_binary_boolean (gimple_stmt_iterator *gsi, + enum tree_code code, + tree op0, tree op1) +{ + gimple op0_def_stmt = SSA_NAME_DEF_STMT (op0); + + if (!is_gimple_assign (op0_def_stmt) + || (gimple_assign_rhs_code (op0_def_stmt) != BIT_NOT_EXPR)) + return false; + + tree x = gimple_assign_rhs1 (op0_def_stmt); + if (TREE_CODE (x) == SSA_NAME + && INTEGRAL_TYPE_P (TREE_TYPE (x)) + && TYPE_PRECISION (TREE_TYPE (x)) == 1 + && TYPE_UNSIGNED (TREE_TYPE (x)) == TYPE_UNSIGNED (TREE_TYPE (op1))) + { + enum tree_code newcode; + + gimple stmt = gsi_stmt (*gsi); + gimple_assign_set_rhs1 (stmt, x); + gimple_assign_set_rhs2 (stmt, op1); + if (code == BIT_AND_EXPR) + newcode = TYPE_UNSIGNED (TREE_TYPE (x)) ? LT_EXPR : GT_EXPR; + else + newcode = TYPE_UNSIGNED (TREE_TYPE (x)) ? LE_EXPR : GE_EXPR; + gimple_assign_set_rhs_code (stmt, newcode); + update_stmt (stmt); + return true; + } + return false; + +} + /* Simplify bitwise binary operations. Return true if a transformation applied, otherwise return false. */ @@ -2117,8 +2163,44 @@ simplify_bitwise_binary (gimple_stmt_iterator *gsi) return true; } } - } + /* If arg1 and arg2 are booleans (or any single bit type) + then try to simplify: + + (~X & Y) -> X < Y + (X & ~Y) -> Y < X + (~X | Y) -> X <= Y + (X | ~Y) -> Y <= X + + But only do this if our result feeds into a comparison as + this transformation is not always a win, particularly on + targets with and-not instructions. */ + if (TREE_CODE (arg1) == SSA_NAME + && TREE_CODE (arg2) == SSA_NAME + && INTEGRAL_TYPE_P (TREE_TYPE (arg1)) + && TYPE_PRECISION (TREE_TYPE (arg1)) == 1 + && TYPE_PRECISION (TREE_TYPE (arg2)) == 1 + && (TYPE_UNSIGNED (TREE_TYPE (arg1)) + == TYPE_UNSIGNED (TREE_TYPE (arg2)))) + { + use_operand_p use_p; + gimple use_stmt; + + if (single_imm_use (gimple_assign_lhs (stmt), &use_p, &use_stmt)) + { + if (gimple_code (use_stmt) == GIMPLE_COND + && gimple_cond_lhs (use_stmt) == gimple_assign_lhs (stmt) + && integer_zerop (gimple_cond_rhs (use_stmt)) + && gimple_cond_code (use_stmt) == NE_EXPR) + { + if (simplify_bitwise_binary_boolean (gsi, code, arg1, arg2)) + return true; + if (simplify_bitwise_binary_boolean (gsi, code, arg2, arg1)) + return true; + } + } + } + } return false; }
The notable changes since the last version: First, it should properly handle signed single bit types, though I haven't tested it with real code. Second, the transformation is only applied when the result is used in a conditional. Thus it's much less likely to pessimize targets with and-not instructions as it's highly likely we'll eliminate two gimple statements rather than just one. Other comments (such as not needing to retrieve gsi_stmt) were also addressed. Testcase was renamed, but is otherwise unchanged. Bootstrapped and regression tested on x86_64-unknown-linux-gnu. OK for the trunk? * tree-ssa-forwprop.c (simplify_bitwise_binary_boolean): New function. (simplify_bitwise_binary): Use it to simpify certain binary ops on booleans. * gcc.dg/tree-ssa/forwprop-28.c: New test.