Message ID | 20231123054758.28829-1-wangfeng@eswincomputing.com |
---|---|
State | New |
Headers | show |
Series | gimple-vr-values:Add constraint for gimple-cond optimization | expand |
On Wed, Nov 22, 2023 at 10:07 PM Feng Wang <wangfeng@eswincomputing.com> wrote: > > This patch add another condition for gimple-cond optimization. Refer to > the following test case. > int foo1 (int data, int res) > { > res = data & 0xf; > res |= res << 4; > if (res < 0x22) > return 0x22; > return res; > } > with the compilation flag "-march=rv64gc_zba_zbb -mabi=lp64d -O2", > before this patch the compilation result is > foo1: > andi a0,a0,15 > slliw a5,a0,4 > addw a3,a5,a0 > li a4,33 > add a0,a5,a0 > bleu a3,a4,.L5 > ret > .L5: > li a0,34 > ret > after this patch the compilation result is > foo1: > andi a0,a0,15 > slliw a5,a0,4 > add a5,a5,a0 > li a0,34 > max a0,a5,a0 > ret > The reason is in the pass_early_vrp, the arg0 of gimple_cond > is replaced,but the PHI node still use the arg0. > The some of evrp pass logs are as follows > gimple_assign <mult_expr, _9, _7, 17, NULL> > gimple_assign <nop_expr, res_5, _9, NULL, NULL> > gimple_cond <le_expr, _9, 33, NULL, NULL> > goto <bb 3>; [INV] > else > goto <bb 4>; [INV] > > <bb 3> : > // predicted unlikely by early return (on trees) predictor. > > <bb 4> : > # gimple_phi <_2, 34(3), res_5(2)> > The arg0 of gimple_cond is replaced by _9,but the gimple_phi still > uses res_5,it will cause optimization fail of PHI node to MAX_EXPR. > So the next_use_is_phi is added to control the replacement. I don't think this is the correct appoarch here. We end up with the same original issue if we had wrote it like: ``` int foo1 (int data, int res) { res = data & 0xf; unsigned int r = res; r*=17; res = r; if (r < 0x22) return 0x22; return res; } ``` I suspect instead we should extend the match.pd patterns to match this max. We should be able to extend: ``` (for cmp (lt le gt ge eq ne) (simplify (cond (cmp (convert1? @1) INTEGER_CST@3) (convert2? @1) INTEGER_CST@2) (with ``` To match instead by changing the second @1 with @4 and then using bitwise_equal_p . If @1 != @4 but bitwise_equal_p is true, you need to make sure the outer convert1/convert2 are nop conversions so that you get the same extension I think ... Note you could instead improve minmax_replacement but I have been in the process of moving those changes to match.pd. Thanks, Andrew Pinski > > gcc/ChangeLog: > > * vr-values.cc (next_use_is_phi): > (simplify_using_ranges::simplify_casted_compare): > add new function next_use_is_phi to control the replacement. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/zbb-min-max-04.c: New test. > --- > gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c | 14 ++++++++++++++ > gcc/vr-values.cc | 15 ++++++++++++++- > 2 files changed, 28 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c > > diff --git a/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c b/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c > new file mode 100644 > index 00000000000..8c3e87a35e0 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c > @@ -0,0 +1,14 @@ > +/* { dg-do compile } */ > +/* { dg-options "-march=rv64gc_zba_zbb -mabi=lp64d" } */ > +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */ > + > +int foo1 (int data, int res) > +{ > + res = data & 0xf; > + res |= res << 4; > + if (res < 0x22) > + return 0x22; > + return res; > +} > + > +/* { dg-final { scan-assembler-times "max\t" 1 } } */ > \ No newline at end of file > diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc > index ecb294131b0..1f7a727c638 100644 > --- a/gcc/vr-values.cc > +++ b/gcc/vr-values.cc > @@ -1263,6 +1263,18 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr > return happened; > } > > +/* Return true if the next use of SSA_NAME is PHI node */ > +bool > +next_use_is_phi (tree arg) > +{ > + use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (arg)); > + use_operand_p next = imm->next; > + if (next && next->loc.stmt > + && (gimple_code (next->loc.stmt) == GIMPLE_PHI)) > + return true; > + return false; > +} > + > /* Simplify OP0 code OP1 when OP1 is a constant and OP0 was a SSA_NAME > defined by a type conversion. Replacing OP0 with RHS of the type conversion. > Doing so makes the conversion dead which helps subsequent passes. */ > @@ -1305,7 +1317,8 @@ simplify_using_ranges::simplify_casted_compare (tree_code &, tree &op0, tree &op > if (TREE_CODE (innerop) == SSA_NAME > && !POINTER_TYPE_P (TREE_TYPE (innerop)) > && !SSA_NAME_OCCURS_IN_ABNORMAL_PHI (innerop) > - && desired_pro_or_demotion_p (TREE_TYPE (innerop), TREE_TYPE (op0))) > + && desired_pro_or_demotion_p (TREE_TYPE (innerop), TREE_TYPE (op0)) > + && !next_use_is_phi (op0)) > { > value_range vr; > > -- > 2.17.1 >
On Wed, Nov 22, 2023 at 10:07 PM Feng Wang <wangfeng@eswincomputing.com> wrote: > > This patch add another condition for gimple-cond optimization. Refer to > the following test case. > int foo1 (int data, int res) > { > res = data & 0xf; > res |= res << 4; > if (res < 0x22) > return 0x22; > return res; > } > with the compilation flag "-march=rv64gc_zba_zbb -mabi=lp64d -O2", > before this patch the compilation result is > foo1: > andi a0,a0,15 > slliw a5,a0,4 > addw a3,a5,a0 > li a4,33 > add a0,a5,a0 > bleu a3,a4,.L5 > ret > .L5: > li a0,34 > ret > after this patch the compilation result is > foo1: > andi a0,a0,15 > slliw a5,a0,4 > add a5,a5,a0 > li a0,34 > max a0,a5,a0 > ret > The reason is in the pass_early_vrp, the arg0 of gimple_cond > is replaced,but the PHI node still use the arg0. > The some of evrp pass logs are as follows > gimple_assign <mult_expr, _9, _7, 17, NULL> > gimple_assign <nop_expr, res_5, _9, NULL, NULL> > gimple_cond <le_expr, _9, 33, NULL, NULL> > goto <bb 3>; [INV] > else > goto <bb 4>; [INV] > > <bb 3> : > // predicted unlikely by early return (on trees) predictor. > > <bb 4> : > # gimple_phi <_2, 34(3), res_5(2)> > The arg0 of gimple_cond is replaced by _9,but the gimple_phi still > uses res_5,it will cause optimization fail of PHI node to MAX_EXPR. > So the next_use_is_phi is added to control the replacement. > > gcc/ChangeLog: > > * vr-values.cc (next_use_is_phi): > (simplify_using_ranges::simplify_casted_compare): > add new function next_use_is_phi to control the replacement. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/zbb-min-max-04.c: New test. One more comment, since this is a generic gimple change, you should add a testcase that is not riscv specific that scans the tree dumps. I would scan phiopt1 in this case to make sure we MAX_EXPR is created early on. Thanks, Andrew Pinski > --- > gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c | 14 ++++++++++++++ > gcc/vr-values.cc | 15 ++++++++++++++- > 2 files changed, 28 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c > > diff --git a/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c b/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c > new file mode 100644 > index 00000000000..8c3e87a35e0 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c > @@ -0,0 +1,14 @@ > +/* { dg-do compile } */ > +/* { dg-options "-march=rv64gc_zba_zbb -mabi=lp64d" } */ > +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */ > + > +int foo1 (int data, int res) > +{ > + res = data & 0xf; > + res |= res << 4; > + if (res < 0x22) > + return 0x22; > + return res; > +} > + > +/* { dg-final { scan-assembler-times "max\t" 1 } } */ > \ No newline at end of file > diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc > index ecb294131b0..1f7a727c638 100644 > --- a/gcc/vr-values.cc > +++ b/gcc/vr-values.cc > @@ -1263,6 +1263,18 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr > return happened; > } > > +/* Return true if the next use of SSA_NAME is PHI node */ > +bool > +next_use_is_phi (tree arg) > +{ > + use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (arg)); > + use_operand_p next = imm->next; > + if (next && next->loc.stmt > + && (gimple_code (next->loc.stmt) == GIMPLE_PHI)) > + return true; > + return false; > +} > + > /* Simplify OP0 code OP1 when OP1 is a constant and OP0 was a SSA_NAME > defined by a type conversion. Replacing OP0 with RHS of the type conversion. > Doing so makes the conversion dead which helps subsequent passes. */ > @@ -1305,7 +1317,8 @@ simplify_using_ranges::simplify_casted_compare (tree_code &, tree &op0, tree &op > if (TREE_CODE (innerop) == SSA_NAME > && !POINTER_TYPE_P (TREE_TYPE (innerop)) > && !SSA_NAME_OCCURS_IN_ABNORMAL_PHI (innerop) > - && desired_pro_or_demotion_p (TREE_TYPE (innerop), TREE_TYPE (op0))) > + && desired_pro_or_demotion_p (TREE_TYPE (innerop), TREE_TYPE (op0)) > + && !next_use_is_phi (op0)) > { > value_range vr; > > -- > 2.17.1 >
On 2023-11-23 14:34 Andrew Pinski<pinskia@gmail.com> wrote: > >On Wed, Nov 22, 2023 at 10:07 PM Feng Wang <wangfeng@eswincomputing.com> wrote: >> >> This patch add another condition for gimple-cond optimization. Refer to >> the following test case. >> int foo1 (int data, int res) >> { >> res = data & 0xf; >> res |= res << 4; >> if (res < 0x22) >> return 0x22; >> return res; >> } >> with the compilation flag "-march=rv64gc_zba_zbb -mabi=lp64d -O2", >> before this patch the compilation result is >> foo1: >> andi a0,a0,15 >> slliw a5,a0,4 >> addw a3,a5,a0 >> li a4,33 >> add a0,a5,a0 >> bleu a3,a4,.L5 >> ret >> .L5: >> li a0,34 >> ret >> after this patch the compilation result is >> foo1: >> andi a0,a0,15 >> slliw a5,a0,4 >> add a5,a5,a0 >> li a0,34 >> max a0,a5,a0 >> ret >> The reason is in the pass_early_vrp, the arg0 of gimple_cond >> is replaced,but the PHI node still use the arg0. >> The some of evrp pass logs are as follows >> gimple_assign <mult_expr, _9, _7, 17, NULL> >> gimple_assign <nop_expr, res_5, _9, NULL, NULL> >> gimple_cond <le_expr, _9, 33, NULL, NULL> >> goto <bb 3>; [INV] >> else >> goto <bb 4>; [INV] >> >> <bb 3> : >> // predicted unlikely by early return (on trees) predictor. >> >> <bb 4> : >> # gimple_phi <_2, 34(3), res_5(2)> >> The arg0 of gimple_cond is replaced by _9,but the gimple_phi still >> uses res_5,it will cause optimization fail of PHI node to MAX_EXPR. >> So the next_use_is_phi is added to control the replacement. > >I don't think this is the correct appoarch here. >We end up with the same original issue if we had wrote it like: >``` >int foo1 (int data, int res) >{ > res = data & 0xf; > unsigned int r = res; > r*=17; > res = r; > if (r < 0x22) > return 0x22; > return res; >} >``` >I suspect instead we should extend the match.pd patterns to match this max. >We should be able to extend: >``` >(for cmp (lt le gt ge eq ne) > (simplify > (cond (cmp (convert1? @1) INTEGER_CST@3) (convert2? @1) INTEGER_CST@2) > (with >``` >To match instead by changing the second @1 with @4 and then using >bitwise_equal_p . If @1 != @4 but bitwise_equal_p is true, you need to >make sure the outer convert1/convert2 are nop conversions so that you >get the same extension I think ... > >Note you could instead improve minmax_replacement but I have been in >the process of moving those changes to match.pd. > >Thanks, >Andrew Pinski Thanks for your feedback. The minmax replacement happens in phiopt pass, there is one condition that requires the "arg_false"(from PHI node) should be same with "smaller"(from gimple_cond). So I made this change. But as you said, this modification is not very suitable, and I have not considered it comprehensively. I'm not very familiar with match.pd, can it solve this judgment problem? Thanks, Feng Wang > >> >> gcc/ChangeLog: >> >> * vr-values.cc (next_use_is_phi): >> (simplify_using_ranges::simplify_casted_compare): >> add new function next_use_is_phi to control the replacement. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/riscv/zbb-min-max-04.c: New test. >> --- >> gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c | 14 ++++++++++++++ >> gcc/vr-values.cc | 15 ++++++++++++++- >> 2 files changed, 28 insertions(+), 1 deletion(-) >> create mode 100644 gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c >> >> diff --git a/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c b/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c >> new file mode 100644 >> index 00000000000..8c3e87a35e0 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c >> @@ -0,0 +1,14 @@ >> +/* { dg-do compile } */ >> +/* { dg-options "-march=rv64gc_zba_zbb -mabi=lp64d" } */ >> +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */ >> + >> +int foo1 (int data, int res) >> +{ >> + res = data & 0xf; >> + res |= res << 4; >> + if (res < 0x22) >> + return 0x22; >> + return res; >> +} >> + >> +/* { dg-final { scan-assembler-times "max\t" 1 } } */ >> \ No newline at end of file >> diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc >> index ecb294131b0..1f7a727c638 100644 >> --- a/gcc/vr-values.cc >> +++ b/gcc/vr-values.cc >> @@ -1263,6 +1263,18 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr >> return happened; >> } >> >> +/* Return true if the next use of SSA_NAME is PHI node */ >> +bool >> +next_use_is_phi (tree arg) >> +{ >> + use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (arg)); >> + use_operand_p next = imm->next; >> + if (next && next->loc.stmt >> + && (gimple_code (next->loc.stmt) == GIMPLE_PHI)) >> + return true; >> + return false; >> +} >> + >> /* Simplify OP0 code OP1 when OP1 is a constant and OP0 was a SSA_NAME >> defined by a type conversion. Replacing OP0 with RHS of the type conversion. >> Doing so makes the conversion dead which helps subsequent passes. */ >> @@ -1305,7 +1317,8 @@ simplify_using_ranges::simplify_casted_compare (tree_code &, tree &op0, tree &op >> if (TREE_CODE (innerop) == SSA_NAME >> && !POINTER_TYPE_P (TREE_TYPE (innerop)) >> && !SSA_NAME_OCCURS_IN_ABNORMAL_PHI (innerop) >> - && desired_pro_or_demotion_p (TREE_TYPE (innerop), TREE_TYPE (op0))) >> + && desired_pro_or_demotion_p (TREE_TYPE (innerop), TREE_TYPE (op0)) >> + && !next_use_is_phi (op0)) >> { >> value_range vr; >> >> -- >> 2.17.1 >>
On 11/22/23 10:47 PM, Feng Wang wrote: > This patch add another condition for gimple-cond optimization. Refer to > the following test case. > int foo1 (int data, int res) > { > res = data & 0xf; > res |= res << 4; > if (res < 0x22) > return 0x22; > return res; > } > with the compilation flag "-march=rv64gc_zba_zbb -mabi=lp64d -O2", > before this patch the compilation result is > foo1: > andi a0,a0,15 > slliw a5,a0,4 > addw a3,a5,a0 > li a4,33 > add a0,a5,a0 > bleu a3,a4,.L5 > ret > .L5: > li a0,34 > ret > after this patch the compilation result is > foo1: > andi a0,a0,15 > slliw a5,a0,4 > add a5,a5,a0 > li a0,34 > max a0,a5,a0 > ret > The reason is in the pass_early_vrp, the arg0 of gimple_cond > is replaced,but the PHI node still use the arg0. > The some of evrp pass logs are as follows > gimple_assign <mult_expr, _9, _7, 17, NULL> > gimple_assign <nop_expr, res_5, _9, NULL, NULL> > gimple_cond <le_expr, _9, 33, NULL, NULL> > goto <bb 3>; [INV] > else > goto <bb 4>; [INV] > > <bb 3> : > // predicted unlikely by early return (on trees) predictor. > > <bb 4> : > # gimple_phi <_2, 34(3), res_5(2)> > The arg0 of gimple_cond is replaced by _9,but the gimple_phi still > uses res_5,it will cause optimization fail of PHI node to MAX_EXPR. > So the next_use_is_phi is added to control the replacement. > > gcc/ChangeLog: > > * vr-values.cc (next_use_is_phi): > (simplify_using_ranges::simplify_casted_compare): > add new function next_use_is_phi to control the replacement. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/zbb-min-max-04.c: New test. I'm not sure what changed, but AFAICT this patch isn't needed anymore. We get the desired code with the trunk compiler. Jeff
diff --git a/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c b/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c new file mode 100644 index 00000000000..8c3e87a35e0 --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gc_zba_zbb -mabi=lp64d" } */ +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */ + +int foo1 (int data, int res) +{ + res = data & 0xf; + res |= res << 4; + if (res < 0x22) + return 0x22; + return res; +} + +/* { dg-final { scan-assembler-times "max\t" 1 } } */ \ No newline at end of file diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc index ecb294131b0..1f7a727c638 100644 --- a/gcc/vr-values.cc +++ b/gcc/vr-values.cc @@ -1263,6 +1263,18 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr return happened; } +/* Return true if the next use of SSA_NAME is PHI node */ +bool +next_use_is_phi (tree arg) +{ + use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (arg)); + use_operand_p next = imm->next; + if (next && next->loc.stmt + && (gimple_code (next->loc.stmt) == GIMPLE_PHI)) + return true; + return false; +} + /* Simplify OP0 code OP1 when OP1 is a constant and OP0 was a SSA_NAME defined by a type conversion. Replacing OP0 with RHS of the type conversion. Doing so makes the conversion dead which helps subsequent passes. */ @@ -1305,7 +1317,8 @@ simplify_using_ranges::simplify_casted_compare (tree_code &, tree &op0, tree &op if (TREE_CODE (innerop) == SSA_NAME && !POINTER_TYPE_P (TREE_TYPE (innerop)) && !SSA_NAME_OCCURS_IN_ABNORMAL_PHI (innerop) - && desired_pro_or_demotion_p (TREE_TYPE (innerop), TREE_TYPE (op0))) + && desired_pro_or_demotion_p (TREE_TYPE (innerop), TREE_TYPE (op0)) + && !next_use_is_phi (op0)) { value_range vr;