| Message ID | patch-14938-tamar@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | middle-end: fix de-optimizations with bitclear patterns on signed values | expand |
On Fri, 15 Oct 2021, Tamar Christina wrote: > Hi All, > > During testing after rebasing to commit I noticed a failing testcase with the > bitmask compare patch. > > Consider the following C++ testcase: > > #include <compare> > > #define A __attribute__((noipa)) > A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0; } > > This turns into a comparison against chars, on systems where chars are signed > the pattern inserts an unsigned convert such that it's able to do the > transformation. > > i.e.: > > # RANGE [-1, 2] > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)> > # RANGE ~[3, 254] > _11 = (unsigned char) c$_M_value_22; > _19 = _11 <= 1; > # .MEM_24 = VDEF <.MEM_6(D)> > D.10434 ={v} {CLOBBER}; > # .MEM_14 = VDEF <.MEM_24> > D.10407 ={v} {CLOBBER}; > # VUSE <.MEM_14> > return _19; > > instead of: > > # RANGE [-1, 2] > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)> > # RANGE [-2, 2] > _3 = c$_M_value_5 & -2; > _19 = _3 == 0; > # .MEM_24 = VDEF <.MEM_6(D)> > D.10440 ={v} {CLOBBER}; > # .MEM_14 = VDEF <.MEM_24> > D.10413 ={v} {CLOBBER}; > # VUSE <.MEM_14> > return _19; > > This causes much worse codegen under -ffast-math due to phiops no longer > recognizing the pattern. It turns out that phiopts spaceship_replacement is > looking for the exact form that was just changed. > > Trying to get it to recognize the new form is not trivial as the transformation > doesn't look to work when the thing it's pointing to is itself a phi-node. What do you mean? Where it handles the BIT_AND it could also handle the conversion, no? The later handling would probably more explicitely need to distinguish between the BIT_AND and the conversion forms. Jakub? > Because of these issues this change delays the replacements until after loop > opts. This fixes the failing C++ testcase. > > Bootstrapped Regtested on aarch64-none-linux-gnu, > x86_64-pc-linux-gnu and no regressions. > > Ok for master? > > Thanks, > Tamar > > gcc/ChangeLog: > > * match.pd: Delay bitmask compare pattern till after loop opts. > > --- inline copy of patch -- > diff --git a/gcc/match.pd b/gcc/match.pd > index 9532cae582e152cae6e22fcce95a9744a844e3c2..d26e498447fc25a327a42cc6a119c6153d09ba03 100644 > --- a/gcc/match.pd > +++ b/gcc/match.pd > @@ -4945,7 +4945,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > icmp (le le gt le gt) > (simplify > (cmp (bit_and:c@2 @0 cst@1) integer_zerop) > - (with { tree csts = bitmask_inv_cst_vector_p (@1); } > + (if (canonicalize_math_after_vectorization_p ()) > + (with { tree csts = bitmask_inv_cst_vector_p (@1); } > (switch > (if (csts && TYPE_UNSIGNED (TREE_TYPE (@1)) > && (VECTOR_TYPE_P (TREE_TYPE (@1)) || single_use (@2))) > @@ -4954,7 +4955,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > && (cmp == EQ_EXPR || cmp == NE_EXPR) > && (VECTOR_TYPE_P (TREE_TYPE (@1)) || single_use (@2))) > (with { tree utype = unsigned_type_for (TREE_TYPE (@1)); } > - (icmp (convert:utype @0) { csts; })))))))) > + (icmp (convert:utype @0) { csts; }))))))))) > > /* -A CMP -B -> B CMP A. */ > (for cmp (tcc_comparison) > > >
> -----Original Message----- > From: Richard Biener <rguenther@suse.de> > Sent: Friday, October 15, 2021 12:31 PM > To: Tamar Christina <Tamar.Christina@arm.com> > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; nd > <nd@arm.com> > Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns > on signed values > > On Fri, 15 Oct 2021, Tamar Christina wrote: > > > Hi All, > > > > During testing after rebasing to commit I noticed a failing testcase > > with the bitmask compare patch. > > > > Consider the following C++ testcase: > > > > #include <compare> > > > > #define A __attribute__((noipa)) > > A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0; } > > > > This turns into a comparison against chars, on systems where chars are > > signed the pattern inserts an unsigned convert such that it's able to > > do the transformation. > > > > i.e.: > > > > # RANGE [-1, 2] > > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)> > > # RANGE ~[3, 254] > > _11 = (unsigned char) c$_M_value_22; > > _19 = _11 <= 1; > > # .MEM_24 = VDEF <.MEM_6(D)> > > D.10434 ={v} {CLOBBER}; > > # .MEM_14 = VDEF <.MEM_24> > > D.10407 ={v} {CLOBBER}; > > # VUSE <.MEM_14> > > return _19; > > > > instead of: > > > > # RANGE [-1, 2] > > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)> > > # RANGE [-2, 2] > > _3 = c$_M_value_5 & -2; > > _19 = _3 == 0; > > # .MEM_24 = VDEF <.MEM_6(D)> > > D.10440 ={v} {CLOBBER}; > > # .MEM_14 = VDEF <.MEM_24> > > D.10413 ={v} {CLOBBER}; > > # VUSE <.MEM_14> > > return _19; > > > > This causes much worse codegen under -ffast-math due to phiops no > > longer recognizing the pattern. It turns out that phiopts > > spaceship_replacement is looking for the exact form that was just changed. > > > > Trying to get it to recognize the new form is not trivial as the > > transformation doesn't look to work when the thing it's pointing to is itself > a phi-node. > > What do you mean? Where it handles the BIT_AND it could also handle the > conversion, no? The later handling would probably more explicitely need to > distinguish between the BIT_AND and the conversion forms. Looks like I misunderstood the code, it was looking at the uses not the defs of the value. --- inline copy of patch --- The comments seems to suggest this code only checks for (res & ~1) == 0 but the implementation seems to suggest it's broader. As such I added a case to check to see if the value comparison we found is a type cast. and strips away the type cast and continues. In match.pd the typecasts are only added for signed comparisons to == 0 and != 0 which are then rewritten into comparisons with 1. As such I only check for 1 and LE and GT, which is what match.pd would have rewritten it to. This fixes the regression but this is not code I 100% understand, since I don't really know the semantics of the spaceship operator so would appreciate an extra look. Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu and no regressions. Ok for master? Thanks, Tamar gcc/ChangeLog: * tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical codegen. diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c index 0e339c46afa29fa97f90d9bc4394370cd9b4b396..65b25be3399b75d5e9cab0f78aa2340418571a33 100644 --- a/gcc/tree-ssa-phiopt.c +++ b/gcc/tree-ssa-phiopt.c @@ -2037,6 +2037,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, tree lhs, rhs; gimple *orig_use_stmt = use_stmt; tree orig_use_lhs = NULL_TREE; + bool is_canon = false; int prec = TYPE_PRECISION (TREE_TYPE (phires)); if (is_gimple_assign (use_stmt) && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR @@ -2063,6 +2064,26 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, } else if (is_gimple_assign (use_stmt)) { + /* Deal with if match.pd has rewritten the (res & ~1) == 0 + into res <= 1 and has left a type-cast for signed types. */ + if (gimple_assign_cast_p (use_stmt)) + { + orig_use_lhs = gimple_assign_lhs (use_stmt); + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) + return false; + if (EDGE_COUNT (phi_bb->preds) != 4) + return false; + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) + return false; + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) + return false; + tree_code cmp; + if (is_gimple_assign (use_stmt) + && (cmp = gimple_assign_rhs_code (use_stmt)) + && (cmp == LE_EXPR || cmp == GT_EXPR) + && wi::eq_p (wi::to_wide (gimple_assign_rhs2 (use_stmt)), 1)) + is_canon = true; + } if (gimple_assign_rhs_class (use_stmt) == GIMPLE_BINARY_RHS) { cmp = gimple_assign_rhs_code (use_stmt); @@ -2099,7 +2120,9 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, || !tree_fits_shwi_p (rhs) || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) return false; - if (orig_use_lhs) + /* If we're already in the canonical form we need to keep the original + comparison. */ + if (orig_use_lhs && !is_canon) { if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) return false; @@ -2310,6 +2333,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, one_cmp = GT_EXPR; enum tree_code res_cmp; + switch (cmp) { case EQ_EXPR: @@ -2345,6 +2369,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; else if (integer_minus_onep (rhs)) res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; + else if (integer_onep (rhs) && is_canon) + res_cmp = GE_EXPR; else return false; break; @@ -2353,6 +2379,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; else if (integer_zerop (rhs)) res_cmp = one_cmp; + else if (integer_onep (rhs) && is_canon) + res_cmp = LE_EXPR; else return false; break;
On Mon, 25 Oct 2021, Tamar Christina wrote: > > -----Original Message----- > > From: Richard Biener <rguenther@suse.de> > > Sent: Friday, October 15, 2021 12:31 PM > > To: Tamar Christina <Tamar.Christina@arm.com> > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; nd > > <nd@arm.com> > > Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns > > on signed values > > > > On Fri, 15 Oct 2021, Tamar Christina wrote: > > > > > Hi All, > > > > > > During testing after rebasing to commit I noticed a failing testcase > > > with the bitmask compare patch. > > > > > > Consider the following C++ testcase: > > > > > > #include <compare> > > > > > > #define A __attribute__((noipa)) > > > A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0; } > > > > > > This turns into a comparison against chars, on systems where chars are > > > signed the pattern inserts an unsigned convert such that it's able to > > > do the transformation. > > > > > > i.e.: > > > > > > # RANGE [-1, 2] > > > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)> > > > # RANGE ~[3, 254] > > > _11 = (unsigned char) c$_M_value_22; > > > _19 = _11 <= 1; > > > # .MEM_24 = VDEF <.MEM_6(D)> > > > D.10434 ={v} {CLOBBER}; > > > # .MEM_14 = VDEF <.MEM_24> > > > D.10407 ={v} {CLOBBER}; > > > # VUSE <.MEM_14> > > > return _19; > > > > > > instead of: > > > > > > # RANGE [-1, 2] > > > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)> > > > # RANGE [-2, 2] > > > _3 = c$_M_value_5 & -2; > > > _19 = _3 == 0; > > > # .MEM_24 = VDEF <.MEM_6(D)> > > > D.10440 ={v} {CLOBBER}; > > > # .MEM_14 = VDEF <.MEM_24> > > > D.10413 ={v} {CLOBBER}; > > > # VUSE <.MEM_14> > > > return _19; > > > > > > This causes much worse codegen under -ffast-math due to phiops no > > > longer recognizing the pattern. It turns out that phiopts > > > spaceship_replacement is looking for the exact form that was just changed. > > > > > > Trying to get it to recognize the new form is not trivial as the > > > transformation doesn't look to work when the thing it's pointing to is itself > > a phi-node. > > > > What do you mean? Where it handles the BIT_AND it could also handle the > > conversion, no? The later handling would probably more explicitely need to > > distinguish between the BIT_AND and the conversion forms. > > Looks like I misunderstood the code, it was looking at the uses not the defs of > the value. > > --- inline copy of patch --- > > The comments seems to suggest this code only checks for (res & ~1) == 0 but the > implementation seems to suggest it's broader. > > As such I added a case to check to see if the value comparison we found is a > type cast. and strips away the type cast and continues. > > In match.pd the typecasts are only added for signed comparisons to == 0 and != 0 > which are then rewritten into comparisons with 1. > > As such I only check for 1 and LE and GT, which is what match.pd would have > rewritten it to. > > This fixes the regression but this is not code I 100% understand, since I don't > really know the semantics of the spaceship operator so would appreciate an extra > look. > > Bootstrapped Regtested on aarch64-none-linux-gnu, > x86_64-pc-linux-gnu and no regressions. > > Ok for master? Please add a testcase. I hope Jakub can review the spaceship_replacement patch since he's the one familiar with the code. Thanks, Richard. > Thanks, > Tamar > > gcc/ChangeLog: > > * tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical > codegen. > > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c > index 0e339c46afa29fa97f90d9bc4394370cd9b4b396..65b25be3399b75d5e9cab0f78aa2340418571a33 100644 > --- a/gcc/tree-ssa-phiopt.c > +++ b/gcc/tree-ssa-phiopt.c > @@ -2037,6 +2037,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > tree lhs, rhs; > gimple *orig_use_stmt = use_stmt; > tree orig_use_lhs = NULL_TREE; > + bool is_canon = false; > int prec = TYPE_PRECISION (TREE_TYPE (phires)); > if (is_gimple_assign (use_stmt) > && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR > @@ -2063,6 +2064,26 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > } > else if (is_gimple_assign (use_stmt)) > { > + /* Deal with if match.pd has rewritten the (res & ~1) == 0 > + into res <= 1 and has left a type-cast for signed types. */ > + if (gimple_assign_cast_p (use_stmt)) > + { > + orig_use_lhs = gimple_assign_lhs (use_stmt); > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) > + return false; > + if (EDGE_COUNT (phi_bb->preds) != 4) > + return false; > + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) > + return false; > + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) > + return false; > + tree_code cmp; > + if (is_gimple_assign (use_stmt) > + && (cmp = gimple_assign_rhs_code (use_stmt)) > + && (cmp == LE_EXPR || cmp == GT_EXPR) > + && wi::eq_p (wi::to_wide (gimple_assign_rhs2 (use_stmt)), 1)) > + is_canon = true; > + } > if (gimple_assign_rhs_class (use_stmt) == GIMPLE_BINARY_RHS) > { > cmp = gimple_assign_rhs_code (use_stmt); > @@ -2099,7 +2120,9 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > || !tree_fits_shwi_p (rhs) > || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) > return false; > - if (orig_use_lhs) > + /* If we're already in the canonical form we need to keep the original > + comparison. */ > + if (orig_use_lhs && !is_canon) > { > if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) > return false; > @@ -2310,6 +2333,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > one_cmp = GT_EXPR; > > enum tree_code res_cmp; > + > switch (cmp) > { > case EQ_EXPR: > @@ -2345,6 +2369,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > else if (integer_minus_onep (rhs)) > res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > + else if (integer_onep (rhs) && is_canon) > + res_cmp = GE_EXPR; > else > return false; > break; > @@ -2353,6 +2379,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > else if (integer_zerop (rhs)) > res_cmp = one_cmp; > + else if (integer_onep (rhs) && is_canon) > + res_cmp = LE_EXPR; > else > return false; > break; >
> -----Original Message----- > From: Richard Biener <rguenther@suse.de> > Sent: Tuesday, October 26, 2021 9:26 AM > To: Tamar Christina <Tamar.Christina@arm.com> > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; nd > <nd@arm.com> > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns > on signed values > > On Mon, 25 Oct 2021, Tamar Christina wrote: > > > > -----Original Message----- > > > From: Richard Biener <rguenther@suse.de> > > > Sent: Friday, October 15, 2021 12:31 PM > > > To: Tamar Christina <Tamar.Christina@arm.com> > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; nd > > > <nd@arm.com> > > > Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear > > > patterns on signed values > > > > > > On Fri, 15 Oct 2021, Tamar Christina wrote: > > > > > > > Hi All, > > > > > > > > During testing after rebasing to commit I noticed a failing > > > > testcase with the bitmask compare patch. > > > > > > > > Consider the following C++ testcase: > > > > > > > > #include <compare> > > > > > > > > #define A __attribute__((noipa)) > > > > A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0; > > > > } > > > > > > > > This turns into a comparison against chars, on systems where chars > > > > are signed the pattern inserts an unsigned convert such that it's > > > > able to do the transformation. > > > > > > > > i.e.: > > > > > > > > # RANGE [-1, 2] > > > > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)> > > > > # RANGE ~[3, 254] > > > > _11 = (unsigned char) c$_M_value_22; > > > > _19 = _11 <= 1; > > > > # .MEM_24 = VDEF <.MEM_6(D)> > > > > D.10434 ={v} {CLOBBER}; > > > > # .MEM_14 = VDEF <.MEM_24> > > > > D.10407 ={v} {CLOBBER}; > > > > # VUSE <.MEM_14> > > > > return _19; > > > > > > > > instead of: > > > > > > > > # RANGE [-1, 2] > > > > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)> > > > > # RANGE [-2, 2] > > > > _3 = c$_M_value_5 & -2; > > > > _19 = _3 == 0; > > > > # .MEM_24 = VDEF <.MEM_6(D)> > > > > D.10440 ={v} {CLOBBER}; > > > > # .MEM_14 = VDEF <.MEM_24> > > > > D.10413 ={v} {CLOBBER}; > > > > # VUSE <.MEM_14> > > > > return _19; > > > > > > > > This causes much worse codegen under -ffast-math due to phiops no > > > > longer recognizing the pattern. It turns out that phiopts > > > > spaceship_replacement is looking for the exact form that was just > changed. > > > > > > > > Trying to get it to recognize the new form is not trivial as the > > > > transformation doesn't look to work when the thing it's pointing > > > > to is itself > > > a phi-node. > > > > > > What do you mean? Where it handles the BIT_AND it could also handle > > > the conversion, no? The later handling would probably more > > > explicitely need to distinguish between the BIT_AND and the conversion > forms. > > > > Looks like I misunderstood the code, it was looking at the uses not > > the defs of the value. > > > > --- inline copy of patch --- > > > > The comments seems to suggest this code only checks for (res & ~1) == > > 0 but the implementation seems to suggest it's broader. > > > > As such I added a case to check to see if the value comparison we > > found is a type cast. and strips away the type cast and continues. > > > > In match.pd the typecasts are only added for signed comparisons to == > > 0 and != 0 which are then rewritten into comparisons with 1. > > > > As such I only check for 1 and LE and GT, which is what match.pd would > > have rewritten it to. > > > > This fixes the regression but this is not code I 100% understand, > > since I don't really know the semantics of the spaceship operator so > > would appreciate an extra look. > > > > Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu > > and no regressions. > > > > Ok for master? > > Please add a testcase. I hope Jakub can review the spaceship_replacement > patch since he's the one familiar with the code. There's already a bunch of testcases that test the various variants: gcc/testsuite/g++.dg/opt/pr94589-1.C and gcc/testsuite/g++.dg/opt/pr94589-2.C which is how I noticed the failure. However they only trigger the failure on signed chars. I tried forcing `-fsigned-char` to see if I can make a general testcase but this seems to have not done it. Is there another flag I can use? Regards, Tamar > > Thanks, > Richard. > > > Thanks, > > Tamar > > > > gcc/ChangeLog: > > > > * tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical > > codegen. > > > > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c index > > > 0e339c46afa29fa97f90d9bc4394370cd9b4b396..65b25be3399b75d5e9cab0f78 > aa2 > > 340418571a33 100644 > > --- a/gcc/tree-ssa-phiopt.c > > +++ b/gcc/tree-ssa-phiopt.c > > @@ -2037,6 +2037,7 @@ spaceship_replacement (basic_block cond_bb, > basic_block middle_bb, > > tree lhs, rhs; > > gimple *orig_use_stmt = use_stmt; > > tree orig_use_lhs = NULL_TREE; > > + bool is_canon = false; > > int prec = TYPE_PRECISION (TREE_TYPE (phires)); > > if (is_gimple_assign (use_stmt) > > && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR @@ -2063,6 > > +2064,26 @@ spaceship_replacement (basic_block cond_bb, basic_block > middle_bb, > > } > > else if (is_gimple_assign (use_stmt)) > > { > > + /* Deal with if match.pd has rewritten the (res & ~1) == 0 > > + into res <= 1 and has left a type-cast for signed types. */ > > + if (gimple_assign_cast_p (use_stmt)) > > + { > > + orig_use_lhs = gimple_assign_lhs (use_stmt); > > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) > > + return false; > > + if (EDGE_COUNT (phi_bb->preds) != 4) > > + return false; > > + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) > > + return false; > > + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) > > + return false; > > + tree_code cmp; > > + if (is_gimple_assign (use_stmt) > > + && (cmp = gimple_assign_rhs_code (use_stmt)) > > + && (cmp == LE_EXPR || cmp == GT_EXPR) > > + && wi::eq_p (wi::to_wide (gimple_assign_rhs2 (use_stmt)), 1)) > > + is_canon = true; > > + } > > if (gimple_assign_rhs_class (use_stmt) == GIMPLE_BINARY_RHS) > > { > > cmp = gimple_assign_rhs_code (use_stmt); @@ -2099,7 +2120,9 > @@ > > spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > > || !tree_fits_shwi_p (rhs) > > || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) > > return false; > > - if (orig_use_lhs) > > + /* If we're already in the canonical form we need to keep the original > > + comparison. */ > > + if (orig_use_lhs && !is_canon) > > { > > if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) > > return false; > > @@ -2310,6 +2333,7 @@ spaceship_replacement (basic_block cond_bb, > basic_block middle_bb, > > one_cmp = GT_EXPR; > > > > enum tree_code res_cmp; > > + > > switch (cmp) > > { > > case EQ_EXPR: > > @@ -2345,6 +2369,8 @@ spaceship_replacement (basic_block cond_bb, > basic_block middle_bb, > > res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > > else if (integer_minus_onep (rhs)) > > res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > > + else if (integer_onep (rhs) && is_canon) > > + res_cmp = GE_EXPR; > > else > > return false; > > break; > > @@ -2353,6 +2379,8 @@ spaceship_replacement (basic_block cond_bb, > basic_block middle_bb, > > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > > else if (integer_zerop (rhs)) > > res_cmp = one_cmp; > > + else if (integer_onep (rhs) && is_canon) > > + res_cmp = LE_EXPR; > > else > > return false; > > break; > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 > Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
On Tue, 26 Oct 2021, Tamar Christina wrote: > > -----Original Message----- > > From: Richard Biener <rguenther@suse.de> > > Sent: Tuesday, October 26, 2021 9:26 AM > > To: Tamar Christina <Tamar.Christina@arm.com> > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; nd > > <nd@arm.com> > > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns > > on signed values > > > > On Mon, 25 Oct 2021, Tamar Christina wrote: > > > > > > -----Original Message----- > > > > From: Richard Biener <rguenther@suse.de> > > > > Sent: Friday, October 15, 2021 12:31 PM > > > > To: Tamar Christina <Tamar.Christina@arm.com> > > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; nd > > > > <nd@arm.com> > > > > Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear > > > > patterns on signed values > > > > > > > > On Fri, 15 Oct 2021, Tamar Christina wrote: > > > > > > > > > Hi All, > > > > > > > > > > During testing after rebasing to commit I noticed a failing > > > > > testcase with the bitmask compare patch. > > > > > > > > > > Consider the following C++ testcase: > > > > > > > > > > #include <compare> > > > > > > > > > > #define A __attribute__((noipa)) > > > > > A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0; > > > > > } > > > > > > > > > > This turns into a comparison against chars, on systems where chars > > > > > are signed the pattern inserts an unsigned convert such that it's > > > > > able to do the transformation. > > > > > > > > > > i.e.: > > > > > > > > > > # RANGE [-1, 2] > > > > > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)> > > > > > # RANGE ~[3, 254] > > > > > _11 = (unsigned char) c$_M_value_22; > > > > > _19 = _11 <= 1; > > > > > # .MEM_24 = VDEF <.MEM_6(D)> > > > > > D.10434 ={v} {CLOBBER}; > > > > > # .MEM_14 = VDEF <.MEM_24> > > > > > D.10407 ={v} {CLOBBER}; > > > > > # VUSE <.MEM_14> > > > > > return _19; > > > > > > > > > > instead of: > > > > > > > > > > # RANGE [-1, 2] > > > > > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)> > > > > > # RANGE [-2, 2] > > > > > _3 = c$_M_value_5 & -2; > > > > > _19 = _3 == 0; > > > > > # .MEM_24 = VDEF <.MEM_6(D)> > > > > > D.10440 ={v} {CLOBBER}; > > > > > # .MEM_14 = VDEF <.MEM_24> > > > > > D.10413 ={v} {CLOBBER}; > > > > > # VUSE <.MEM_14> > > > > > return _19; > > > > > > > > > > This causes much worse codegen under -ffast-math due to phiops no > > > > > longer recognizing the pattern. It turns out that phiopts > > > > > spaceship_replacement is looking for the exact form that was just > > changed. > > > > > > > > > > Trying to get it to recognize the new form is not trivial as the > > > > > transformation doesn't look to work when the thing it's pointing > > > > > to is itself > > > > a phi-node. > > > > > > > > What do you mean? Where it handles the BIT_AND it could also handle > > > > the conversion, no? The later handling would probably more > > > > explicitely need to distinguish between the BIT_AND and the conversion > > forms. > > > > > > Looks like I misunderstood the code, it was looking at the uses not > > > the defs of the value. > > > > > > --- inline copy of patch --- > > > > > > The comments seems to suggest this code only checks for (res & ~1) == > > > 0 but the implementation seems to suggest it's broader. > > > > > > As such I added a case to check to see if the value comparison we > > > found is a type cast. and strips away the type cast and continues. > > > > > > In match.pd the typecasts are only added for signed comparisons to == > > > 0 and != 0 which are then rewritten into comparisons with 1. > > > > > > As such I only check for 1 and LE and GT, which is what match.pd would > > > have rewritten it to. > > > > > > This fixes the regression but this is not code I 100% understand, > > > since I don't really know the semantics of the spaceship operator so > > > would appreciate an extra look. > > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu > > > and no regressions. > > > > > > Ok for master? > > > > Please add a testcase. I hope Jakub can review the spaceship_replacement > > patch since he's the one familiar with the code. > > There's already a bunch of testcases that test the various variants: gcc/testsuite/g++.dg/opt/pr94589-1.C > and gcc/testsuite/g++.dg/opt/pr94589-2.C which is how I noticed the failure. However they only trigger > the failure on signed chars. I tried forcing `-fsigned-char` to see if I can make a general testcase but this > seems to have not done it. > > Is there another flag I can use? I suppose you can copy the testcase(s) and replace 'auto' with 'signed char'? > > Regards, > Tamar > > > > Thanks, > > Richard. > > > > > Thanks, > > > Tamar > > > > > > gcc/ChangeLog: > > > > > > * tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical > > > codegen. > > > > > > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c index > > > > > 0e339c46afa29fa97f90d9bc4394370cd9b4b396..65b25be3399b75d5e9cab0f78 > > aa2 > > > 340418571a33 100644 > > > --- a/gcc/tree-ssa-phiopt.c > > > +++ b/gcc/tree-ssa-phiopt.c > > > @@ -2037,6 +2037,7 @@ spaceship_replacement (basic_block cond_bb, > > basic_block middle_bb, > > > tree lhs, rhs; > > > gimple *orig_use_stmt = use_stmt; > > > tree orig_use_lhs = NULL_TREE; > > > + bool is_canon = false; > > > int prec = TYPE_PRECISION (TREE_TYPE (phires)); > > > if (is_gimple_assign (use_stmt) > > > && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR @@ -2063,6 > > > +2064,26 @@ spaceship_replacement (basic_block cond_bb, basic_block > > middle_bb, > > > } > > > else if (is_gimple_assign (use_stmt)) > > > { > > > + /* Deal with if match.pd has rewritten the (res & ~1) == 0 > > > + into res <= 1 and has left a type-cast for signed types. */ > > > + if (gimple_assign_cast_p (use_stmt)) > > > + { > > > + orig_use_lhs = gimple_assign_lhs (use_stmt); > > > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) > > > + return false; > > > + if (EDGE_COUNT (phi_bb->preds) != 4) > > > + return false; > > > + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) > > > + return false; > > > + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) > > > + return false; > > > + tree_code cmp; > > > + if (is_gimple_assign (use_stmt) > > > + && (cmp = gimple_assign_rhs_code (use_stmt)) > > > + && (cmp == LE_EXPR || cmp == GT_EXPR) > > > + && wi::eq_p (wi::to_wide (gimple_assign_rhs2 (use_stmt)), 1)) > > > + is_canon = true; > > > + } > > > if (gimple_assign_rhs_class (use_stmt) == GIMPLE_BINARY_RHS) > > > { > > > cmp = gimple_assign_rhs_code (use_stmt); @@ -2099,7 +2120,9 > > @@ > > > spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > > > || !tree_fits_shwi_p (rhs) > > > || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) > > > return false; > > > - if (orig_use_lhs) > > > + /* If we're already in the canonical form we need to keep the original > > > + comparison. */ > > > + if (orig_use_lhs && !is_canon) > > > { > > > if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) > > > return false; > > > @@ -2310,6 +2333,7 @@ spaceship_replacement (basic_block cond_bb, > > basic_block middle_bb, > > > one_cmp = GT_EXPR; > > > > > > enum tree_code res_cmp; > > > + > > > switch (cmp) > > > { > > > case EQ_EXPR: > > > @@ -2345,6 +2369,8 @@ spaceship_replacement (basic_block cond_bb, > > basic_block middle_bb, > > > res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > > > else if (integer_minus_onep (rhs)) > > > res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > > > + else if (integer_onep (rhs) && is_canon) > > > + res_cmp = GE_EXPR; > > > else > > > return false; > > > break; > > > @@ -2353,6 +2379,8 @@ spaceship_replacement (basic_block cond_bb, > > basic_block middle_bb, > > > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > > > else if (integer_zerop (rhs)) > > > res_cmp = one_cmp; > > > + else if (integer_onep (rhs) && is_canon) > > > + res_cmp = LE_EXPR; > > > else > > > return false; > > > break; > > > > > > > -- > > Richard Biener <rguenther@suse.de> > > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 > > Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg) >
> -----Original Message----- > From: Richard Biener <rguenther@suse.de> > Sent: Tuesday, October 26, 2021 9:46 AM > To: Tamar Christina <Tamar.Christina@arm.com> > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; nd > <nd@arm.com> > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns > on signed values > > On Tue, 26 Oct 2021, Tamar Christina wrote: > > > > -----Original Message----- > > > From: Richard Biener <rguenther@suse.de> > > > Sent: Tuesday, October 26, 2021 9:26 AM > > > To: Tamar Christina <Tamar.Christina@arm.com> > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; nd > > > <nd@arm.com> > > > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear > > > patterns on signed values > > > > > > On Mon, 25 Oct 2021, Tamar Christina wrote: > > > > > > > > -----Original Message----- > > > > > From: Richard Biener <rguenther@suse.de> > > > > > Sent: Friday, October 15, 2021 12:31 PM > > > > > To: Tamar Christina <Tamar.Christina@arm.com> > > > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; > > > > > nd <nd@arm.com> > > > > > Subject: Re: [PATCH] middle-end: fix de-optimizations with > > > > > bitclear patterns on signed values > > > > > > > > > > On Fri, 15 Oct 2021, Tamar Christina wrote: > > > > > > > > > > > Hi All, > > > > > > > > > > > > During testing after rebasing to commit I noticed a failing > > > > > > testcase with the bitmask compare patch. > > > > > > > > > > > > Consider the following C++ testcase: > > > > > > > > > > > > #include <compare> > > > > > > > > > > > > #define A __attribute__((noipa)) A bool f5 (double i, double > > > > > > j) { auto c = i <=> j; return c >= 0; } > > > > > > > > > > > > This turns into a comparison against chars, on systems where > > > > > > chars are signed the pattern inserts an unsigned convert such > > > > > > that it's able to do the transformation. > > > > > > > > > > > > i.e.: > > > > > > > > > > > > # RANGE [-1, 2] > > > > > > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)> > > > > > > # RANGE ~[3, 254] > > > > > > _11 = (unsigned char) c$_M_value_22; > > > > > > _19 = _11 <= 1; > > > > > > # .MEM_24 = VDEF <.MEM_6(D)> > > > > > > D.10434 ={v} {CLOBBER}; > > > > > > # .MEM_14 = VDEF <.MEM_24> > > > > > > D.10407 ={v} {CLOBBER}; > > > > > > # VUSE <.MEM_14> > > > > > > return _19; > > > > > > > > > > > > instead of: > > > > > > > > > > > > # RANGE [-1, 2] > > > > > > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)> > > > > > > # RANGE [-2, 2] > > > > > > _3 = c$_M_value_5 & -2; > > > > > > _19 = _3 == 0; > > > > > > # .MEM_24 = VDEF <.MEM_6(D)> > > > > > > D.10440 ={v} {CLOBBER}; > > > > > > # .MEM_14 = VDEF <.MEM_24> > > > > > > D.10413 ={v} {CLOBBER}; > > > > > > # VUSE <.MEM_14> > > > > > > return _19; > > > > > > > > > > > > This causes much worse codegen under -ffast-math due to phiops > > > > > > no longer recognizing the pattern. It turns out that phiopts > > > > > > spaceship_replacement is looking for the exact form that was > > > > > > just > > > changed. > > > > > > > > > > > > Trying to get it to recognize the new form is not trivial as > > > > > > the transformation doesn't look to work when the thing it's > > > > > > pointing to is itself > > > > > a phi-node. > > > > > > > > > > What do you mean? Where it handles the BIT_AND it could also > > > > > handle the conversion, no? The later handling would probably > > > > > more explicitely need to distinguish between the BIT_AND and the > > > > > conversion > > > forms. > > > > > > > > Looks like I misunderstood the code, it was looking at the uses > > > > not the defs of the value. > > > > > > > > --- inline copy of patch --- > > > > > > > > The comments seems to suggest this code only checks for (res & ~1) > > > > == > > > > 0 but the implementation seems to suggest it's broader. > > > > > > > > As such I added a case to check to see if the value comparison we > > > > found is a type cast. and strips away the type cast and continues. > > > > > > > > In match.pd the typecasts are only added for signed comparisons to > > > > == > > > > 0 and != 0 which are then rewritten into comparisons with 1. > > > > > > > > As such I only check for 1 and LE and GT, which is what match.pd > > > > would have rewritten it to. > > > > > > > > This fixes the regression but this is not code I 100% understand, > > > > since I don't really know the semantics of the spaceship operator > > > > so would appreciate an extra look. > > > > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu, > > > > x86_64-pc-linux-gnu and no regressions. > > > > > > > > Ok for master? > > > > > > Please add a testcase. I hope Jakub can review the > > > spaceship_replacement patch since he's the one familiar with the code. > > > > There's already a bunch of testcases that test the various variants: > > gcc/testsuite/g++.dg/opt/pr94589-1.C > > and gcc/testsuite/g++.dg/opt/pr94589-2.C which is how I noticed the > > failure. However they only trigger the failure on signed chars. I > > tried forcing `-fsigned-char` to see if I can make a general testcase but this > seems to have not done it. > > > > Is there another flag I can use? > > I suppose you can copy the testcase(s) and replace 'auto' with 'signed char'? Unfortunately that dies with bit.cc: In function 'bool f5(double, double)': bit.cc:4:52: error: cannot convert 'std::partial_ordering' to 'signed char' in initialization 4 | A bool f5 (double i, double j) { signed char c = i <=> j; return c >= 0; } | ~~^~~~~ | | | std::partial_ordering It looks like the chars end up there after inlining the `<=>` operation itself. I could do a Gimple testcase, but worry it may be a bit fragile. Regards, Tamar. > > > > > Regards, > > Tamar > > > > > > Thanks, > > > Richard. > > > > > > > Thanks, > > > > Tamar > > > > > > > > gcc/ChangeLog: > > > > > > > > * tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical > > > > codegen. > > > > > > > > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c index > > > > > > > > 0e339c46afa29fa97f90d9bc4394370cd9b4b396..65b25be3399b75d5e9cab0f78 > > > aa2 > > > > 340418571a33 100644 > > > > --- a/gcc/tree-ssa-phiopt.c > > > > +++ b/gcc/tree-ssa-phiopt.c > > > > @@ -2037,6 +2037,7 @@ spaceship_replacement (basic_block cond_bb, > > > basic_block middle_bb, > > > > tree lhs, rhs; > > > > gimple *orig_use_stmt = use_stmt; > > > > tree orig_use_lhs = NULL_TREE; > > > > + bool is_canon = false; > > > > int prec = TYPE_PRECISION (TREE_TYPE (phires)); > > > > if (is_gimple_assign (use_stmt) > > > > && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR @@ > > > > -2063,6 > > > > +2064,26 @@ spaceship_replacement (basic_block cond_bb, > > > > +basic_block > > > middle_bb, > > > > } > > > > else if (is_gimple_assign (use_stmt)) > > > > { > > > > + /* Deal with if match.pd has rewritten the (res & ~1) == 0 > > > > + into res <= 1 and has left a type-cast for signed types. */ > > > > + if (gimple_assign_cast_p (use_stmt)) > > > > + { > > > > + orig_use_lhs = gimple_assign_lhs (use_stmt); > > > > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) > > > > + return false; > > > > + if (EDGE_COUNT (phi_bb->preds) != 4) > > > > + return false; > > > > + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) > > > > + return false; > > > > + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) > > > > + return false; > > > > + tree_code cmp; > > > > + if (is_gimple_assign (use_stmt) > > > > + && (cmp = gimple_assign_rhs_code (use_stmt)) > > > > + && (cmp == LE_EXPR || cmp == GT_EXPR) > > > > + && wi::eq_p (wi::to_wide (gimple_assign_rhs2 (use_stmt)), 1)) > > > > + is_canon = true; > > > > + } > > > > if (gimple_assign_rhs_class (use_stmt) == GIMPLE_BINARY_RHS) > > > > { > > > > cmp = gimple_assign_rhs_code (use_stmt); @@ -2099,7 +2120,9 > > > @@ > > > > spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > > > > || !tree_fits_shwi_p (rhs) > > > > || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) > > > > return false; > > > > - if (orig_use_lhs) > > > > + /* If we're already in the canonical form we need to keep the original > > > > + comparison. */ > > > > + if (orig_use_lhs && !is_canon) > > > > { > > > > if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) > > > > return false; > > > > @@ -2310,6 +2333,7 @@ spaceship_replacement (basic_block cond_bb, > > > basic_block middle_bb, > > > > one_cmp = GT_EXPR; > > > > > > > > enum tree_code res_cmp; > > > > + > > > > switch (cmp) > > > > { > > > > case EQ_EXPR: > > > > @@ -2345,6 +2369,8 @@ spaceship_replacement (basic_block cond_bb, > > > basic_block middle_bb, > > > > res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > > > > else if (integer_minus_onep (rhs)) > > > > res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > > > > + else if (integer_onep (rhs) && is_canon) > > > > + res_cmp = GE_EXPR; > > > > else > > > > return false; > > > > break; > > > > @@ -2353,6 +2379,8 @@ spaceship_replacement (basic_block cond_bb, > > > basic_block middle_bb, > > > > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > > > > else if (integer_zerop (rhs)) > > > > res_cmp = one_cmp; > > > > + else if (integer_onep (rhs) && is_canon) > > > > + res_cmp = LE_EXPR; > > > > else > > > > return false; > > > > break; > > > > > > > > > > -- > > > Richard Biener <rguenther@suse.de> > > > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 > > > Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg) > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 > Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
On Tue, 26 Oct 2021, Tamar Christina wrote: > > > > -----Original Message----- > > From: Richard Biener <rguenther@suse.de> > > Sent: Tuesday, October 26, 2021 9:46 AM > > To: Tamar Christina <Tamar.Christina@arm.com> > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; nd > > <nd@arm.com> > > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns > > on signed values > > > > On Tue, 26 Oct 2021, Tamar Christina wrote: > > > > > > -----Original Message----- > > > > From: Richard Biener <rguenther@suse.de> > > > > Sent: Tuesday, October 26, 2021 9:26 AM > > > > To: Tamar Christina <Tamar.Christina@arm.com> > > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; nd > > > > <nd@arm.com> > > > > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear > > > > patterns on signed values > > > > > > > > On Mon, 25 Oct 2021, Tamar Christina wrote: > > > > > > > > > > -----Original Message----- > > > > > > From: Richard Biener <rguenther@suse.de> > > > > > > Sent: Friday, October 15, 2021 12:31 PM > > > > > > To: Tamar Christina <Tamar.Christina@arm.com> > > > > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; > > > > > > nd <nd@arm.com> > > > > > > Subject: Re: [PATCH] middle-end: fix de-optimizations with > > > > > > bitclear patterns on signed values > > > > > > > > > > > > On Fri, 15 Oct 2021, Tamar Christina wrote: > > > > > > > > > > > > > Hi All, > > > > > > > > > > > > > > During testing after rebasing to commit I noticed a failing > > > > > > > testcase with the bitmask compare patch. > > > > > > > > > > > > > > Consider the following C++ testcase: > > > > > > > > > > > > > > #include <compare> > > > > > > > > > > > > > > #define A __attribute__((noipa)) A bool f5 (double i, double > > > > > > > j) { auto c = i <=> j; return c >= 0; } > > > > > > > > > > > > > > This turns into a comparison against chars, on systems where > > > > > > > chars are signed the pattern inserts an unsigned convert such > > > > > > > that it's able to do the transformation. > > > > > > > > > > > > > > i.e.: > > > > > > > > > > > > > > # RANGE [-1, 2] > > > > > > > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)> > > > > > > > # RANGE ~[3, 254] > > > > > > > _11 = (unsigned char) c$_M_value_22; > > > > > > > _19 = _11 <= 1; > > > > > > > # .MEM_24 = VDEF <.MEM_6(D)> > > > > > > > D.10434 ={v} {CLOBBER}; > > > > > > > # .MEM_14 = VDEF <.MEM_24> > > > > > > > D.10407 ={v} {CLOBBER}; > > > > > > > # VUSE <.MEM_14> > > > > > > > return _19; > > > > > > > > > > > > > > instead of: > > > > > > > > > > > > > > # RANGE [-1, 2] > > > > > > > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)> > > > > > > > # RANGE [-2, 2] > > > > > > > _3 = c$_M_value_5 & -2; > > > > > > > _19 = _3 == 0; > > > > > > > # .MEM_24 = VDEF <.MEM_6(D)> > > > > > > > D.10440 ={v} {CLOBBER}; > > > > > > > # .MEM_14 = VDEF <.MEM_24> > > > > > > > D.10413 ={v} {CLOBBER}; > > > > > > > # VUSE <.MEM_14> > > > > > > > return _19; > > > > > > > > > > > > > > This causes much worse codegen under -ffast-math due to phiops > > > > > > > no longer recognizing the pattern. It turns out that phiopts > > > > > > > spaceship_replacement is looking for the exact form that was > > > > > > > just > > > > changed. > > > > > > > > > > > > > > Trying to get it to recognize the new form is not trivial as > > > > > > > the transformation doesn't look to work when the thing it's > > > > > > > pointing to is itself > > > > > > a phi-node. > > > > > > > > > > > > What do you mean? Where it handles the BIT_AND it could also > > > > > > handle the conversion, no? The later handling would probably > > > > > > more explicitely need to distinguish between the BIT_AND and the > > > > > > conversion > > > > forms. > > > > > > > > > > Looks like I misunderstood the code, it was looking at the uses > > > > > not the defs of the value. > > > > > > > > > > --- inline copy of patch --- > > > > > > > > > > The comments seems to suggest this code only checks for (res & ~1) > > > > > == > > > > > 0 but the implementation seems to suggest it's broader. > > > > > > > > > > As such I added a case to check to see if the value comparison we > > > > > found is a type cast. and strips away the type cast and continues. > > > > > > > > > > In match.pd the typecasts are only added for signed comparisons to > > > > > == > > > > > 0 and != 0 which are then rewritten into comparisons with 1. > > > > > > > > > > As such I only check for 1 and LE and GT, which is what match.pd > > > > > would have rewritten it to. > > > > > > > > > > This fixes the regression but this is not code I 100% understand, > > > > > since I don't really know the semantics of the spaceship operator > > > > > so would appreciate an extra look. > > > > > > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu, > > > > > x86_64-pc-linux-gnu and no regressions. > > > > > > > > > > Ok for master? > > > > > > > > Please add a testcase. I hope Jakub can review the > > > > spaceship_replacement patch since he's the one familiar with the code. > > > > > > There's already a bunch of testcases that test the various variants: > > > gcc/testsuite/g++.dg/opt/pr94589-1.C > > > and gcc/testsuite/g++.dg/opt/pr94589-2.C which is how I noticed the > > > failure. However they only trigger the failure on signed chars. I > > > tried forcing `-fsigned-char` to see if I can make a general testcase but this > > seems to have not done it. > > > > > > Is there another flag I can use? > > > > I suppose you can copy the testcase(s) and replace 'auto' with 'signed char'? > > Unfortunately that dies with > > bit.cc: In function 'bool f5(double, double)': > bit.cc:4:52: error: cannot convert 'std::partial_ordering' to 'signed char' in initialization > 4 | A bool f5 (double i, double j) { signed char c = i <=> j; return c >= 0; } > | ~~^~~~~ > | | > | std::partial_ordering > > It looks like the chars end up there after inlining the `<=>` operation itself. > I could do a Gimple testcase, but worry it may be a bit fragile. try auto c = ...; signed char c2 = c; return c2 >= ... then > Regards, > Tamar. > > > > > > > > > Regards, > > > Tamar > > > > > > > > Thanks, > > > > Richard. > > > > > > > > > Thanks, > > > > > Tamar > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > * tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical > > > > > codegen. > > > > > > > > > > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c index > > > > > > > > > > > 0e339c46afa29fa97f90d9bc4394370cd9b4b396..65b25be3399b75d5e9cab0f78 > > > > aa2 > > > > > 340418571a33 100644 > > > > > --- a/gcc/tree-ssa-phiopt.c > > > > > +++ b/gcc/tree-ssa-phiopt.c > > > > > @@ -2037,6 +2037,7 @@ spaceship_replacement (basic_block cond_bb, > > > > basic_block middle_bb, > > > > > tree lhs, rhs; > > > > > gimple *orig_use_stmt = use_stmt; > > > > > tree orig_use_lhs = NULL_TREE; > > > > > + bool is_canon = false; > > > > > int prec = TYPE_PRECISION (TREE_TYPE (phires)); > > > > > if (is_gimple_assign (use_stmt) > > > > > && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR @@ > > > > > -2063,6 > > > > > +2064,26 @@ spaceship_replacement (basic_block cond_bb, > > > > > +basic_block > > > > middle_bb, > > > > > } > > > > > else if (is_gimple_assign (use_stmt)) > > > > > { > > > > > + /* Deal with if match.pd has rewritten the (res & ~1) == 0 > > > > > + into res <= 1 and has left a type-cast for signed types. */ > > > > > + if (gimple_assign_cast_p (use_stmt)) > > > > > + { > > > > > + orig_use_lhs = gimple_assign_lhs (use_stmt); > > > > > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) > > > > > + return false; > > > > > + if (EDGE_COUNT (phi_bb->preds) != 4) > > > > > + return false; > > > > > + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) > > > > > + return false; > > > > > + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) > > > > > + return false; > > > > > + tree_code cmp; > > > > > + if (is_gimple_assign (use_stmt) > > > > > + && (cmp = gimple_assign_rhs_code (use_stmt)) > > > > > + && (cmp == LE_EXPR || cmp == GT_EXPR) > > > > > + && wi::eq_p (wi::to_wide (gimple_assign_rhs2 (use_stmt)), 1)) > > > > > + is_canon = true; > > > > > + } > > > > > if (gimple_assign_rhs_class (use_stmt) == GIMPLE_BINARY_RHS) > > > > > { > > > > > cmp = gimple_assign_rhs_code (use_stmt); @@ -2099,7 +2120,9 > > > > @@ > > > > > spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > > > > > || !tree_fits_shwi_p (rhs) > > > > > || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) > > > > > return false; > > > > > - if (orig_use_lhs) > > > > > + /* If we're already in the canonical form we need to keep the original > > > > > + comparison. */ > > > > > + if (orig_use_lhs && !is_canon) > > > > > { > > > > > if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) > > > > > return false; > > > > > @@ -2310,6 +2333,7 @@ spaceship_replacement (basic_block cond_bb, > > > > basic_block middle_bb, > > > > > one_cmp = GT_EXPR; > > > > > > > > > > enum tree_code res_cmp; > > > > > + > > > > > switch (cmp) > > > > > { > > > > > case EQ_EXPR: > > > > > @@ -2345,6 +2369,8 @@ spaceship_replacement (basic_block cond_bb, > > > > basic_block middle_bb, > > > > > res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > > > > > else if (integer_minus_onep (rhs)) > > > > > res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > > > > > + else if (integer_onep (rhs) && is_canon) > > > > > + res_cmp = GE_EXPR; > > > > > else > > > > > return false; > > > > > break; > > > > > @@ -2353,6 +2379,8 @@ spaceship_replacement (basic_block cond_bb, > > > > basic_block middle_bb, > > > > > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > > > > > else if (integer_zerop (rhs)) > > > > > res_cmp = one_cmp; > > > > > + else if (integer_onep (rhs) && is_canon) > > > > > + res_cmp = LE_EXPR; > > > > > else > > > > > return false; > > > > > break; > > > > > > > > > > > > > -- > > > > Richard Biener <rguenther@suse.de> > > > > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 > > > > Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg) > > > > > > > -- > > Richard Biener <rguenther@suse.de> > > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 > > Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg) >
On Tue, Oct 26, 2021 at 03:13:29PM +0200, Richard Biener wrote: > try > auto c = ...; > signed char c2 = c; > return c2 >= ... > then That won't work, at least when using <compare>, which is what we with the optimization want to deal with primarily. Because std::partial_ordering etc. aren't implicitly nor explicitly convertible to int or signed char etc. Sure, one could in the testcase define its own std::strong_ordering etc. and define a conversion operator for it... Jakub
On Tue, 26 Oct 2021, Jakub Jelinek wrote: > On Tue, Oct 26, 2021 at 03:13:29PM +0200, Richard Biener wrote: > > try > > auto c = ...; > > signed char c2 = c; > > return c2 >= ... > > then > > That won't work, at least when using <compare>, which is what we with the > optimization want to deal with primarily. > Because std::partial_ordering etc. aren't implicitly nor explicitly > convertible to int or signed char etc. > Sure, one could in the testcase define its own std::strong_ordering etc. > and define a conversion operator for it... So how do we end up with the signed char case in the first place? Is the frontend using a type that's target dependent? Richard.
On Tue, Oct 26, 2021 at 03:21:55PM +0200, Richard Biener wrote: > On Tue, 26 Oct 2021, Jakub Jelinek wrote: > > > On Tue, Oct 26, 2021 at 03:13:29PM +0200, Richard Biener wrote: > > > try > > > auto c = ...; > > > signed char c2 = c; > > > return c2 >= ... > > > then > > > > That won't work, at least when using <compare>, which is what we with the > > optimization want to deal with primarily. > > Because std::partial_ordering etc. aren't implicitly nor explicitly > > convertible to int or signed char etc. > > Sure, one could in the testcase define its own std::strong_ordering etc. > > and define a conversion operator for it... > > So how do we end up with the signed char case in the first place? > Is the frontend using a type that's target dependent? <compare> uses explicitly signed char: namespace std { // [cmp.categories], comparison category types namespace __cmp_cat { using type = signed char; enum class _Ord : type { equivalent = 0, less = -1, greater = 1 }; enum class _Ncmp : type { _Unordered = 2 }; ... and __cmp_cat::type is what is used as type of _M_value of std::*_ordering -fsigned-char vs. -funsigned-char make no difference on the testcases on x86, but as mentioned in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94589#c24 some target decisions like load_extend_op uses in fold-const.c can affect it. See https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570714.html Jakub
On Tue, 26 Oct 2021, Jakub Jelinek wrote: > On Tue, Oct 26, 2021 at 03:21:55PM +0200, Richard Biener wrote: > > On Tue, 26 Oct 2021, Jakub Jelinek wrote: > > > > > On Tue, Oct 26, 2021 at 03:13:29PM +0200, Richard Biener wrote: > > > > try > > > > auto c = ...; > > > > signed char c2 = c; > > > > return c2 >= ... > > > > then > > > > > > That won't work, at least when using <compare>, which is what we with the > > > optimization want to deal with primarily. > > > Because std::partial_ordering etc. aren't implicitly nor explicitly > > > convertible to int or signed char etc. > > > Sure, one could in the testcase define its own std::strong_ordering etc. > > > and define a conversion operator for it... > > > > So how do we end up with the signed char case in the first place? > > Is the frontend using a type that's target dependent? > > <compare> uses explicitly signed char: > namespace std > { > // [cmp.categories], comparison category types > namespace __cmp_cat > { > using type = signed char; > enum class _Ord : type { equivalent = 0, less = -1, greater = 1 }; > enum class _Ncmp : type { _Unordered = 2 }; > ... > and __cmp_cat::type is what is used as type of _M_value of std::*_ordering > -fsigned-char vs. -funsigned-char make no difference on the testcases on > x86, but as mentioned in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94589#c24 > some target decisions like load_extend_op uses in fold-const.c can affect > it. See https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570714.html Eh, I see. So there are alrady testcases covering the issues on the affected targets. So ignore my comment about adding additional testcases. Richard.
On Tue, 26 Oct 2021 at 14:36, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Oct 26, 2021 at 03:21:55PM +0200, Richard Biener wrote: > > On Tue, 26 Oct 2021, Jakub Jelinek wrote: > > > > > On Tue, Oct 26, 2021 at 03:13:29PM +0200, Richard Biener wrote: > > > > try > > > > auto c = ...; > > > > signed char c2 = c; > > > > return c2 >= ... > > > > then > > > > > > That won't work, at least when using <compare>, which is what we with > the > > > optimization want to deal with primarily. > > > Because std::partial_ordering etc. aren't implicitly nor explicitly > > > convertible to int or signed char etc. > > > Sure, one could in the testcase define its own std::strong_ordering > etc. > > > and define a conversion operator for it... > > > > So how do we end up with the signed char case in the first place? > > Is the frontend using a type that's target dependent? > > <compare> uses explicitly signed char: > namespace std > { > // [cmp.categories], comparison category types > namespace __cmp_cat > { > using type = signed char; > enum class _Ord : type { equivalent = 0, less = -1, greater = 1 }; > enum class _Ncmp : type { _Unordered = 2 }; > ... > and __cmp_cat::type is what is used as type of _M_value of std::*_ordering > -fsigned-char vs. -funsigned-char make no difference on the testcases on > x86, but as mentioned in > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94589#c24 > some target decisions like load_extend_op uses in fold-const.c can affect > it. See https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570714.html > > We can change __cmp_cat::type if that would result in better code. I picked signed char because we only need two bits, and preferably have a signed type as it simplifies some things. Would int make more sense? Or int:2 ?
On Tue, Oct 26, 2021 at 08:35:40PM +0100, Jonathan Wakely wrote: > We can change __cmp_cat::type if that would result in better code. I picked > signed char because we only need two bits, and preferably have a signed > type as it simplifies some things. Would int make more sense? Or int:2 ? I think signed char is fine (and changing it would be an ABI change), int is unnecessarily large and int:2 would be certainly slower (and harder). Jakub
On Tue, 26 Oct 2021 at 20:40, Jakub Jelinek wrote: > On Tue, Oct 26, 2021 at 08:35:40PM +0100, Jonathan Wakely wrote: > > We can change __cmp_cat::type if that would result in better code. I > picked > > signed char because we only need two bits, and preferably have a signed > > type as it simplifies some things. Would int make more sense? Or int:2 ? > > I think signed char is fine (and changing it would be an ABI change), We haven't committed to a C++20 ABI yet, so changes are possible. If we don't need to change, that's obviously preferable. > int is > unnecessarily large and int:2 would be certainly slower (and harder). > OK, signed char it is then.
Hi,
I think it was lost along the way that I did post an update to the detection code to fix the regression 😊
I think I have a better understanding of the code now and have updated the patch.
Essentially when a signed comparison is encountered my match.pd pattern will trigger for
EQ and NE. It rewrites these by inserting an unsigned cast and does a unsigned comparison
With a modified immediate.
The spaceship operator is looking for (res & 1) == res which was previously folded to (res & ~1) == 0.
This is now being folded further to ((unsigned) res) <= 1.
Given this this patch adds additional checks for when the value is an unsigned type conversion
and the comparison value on the rhs is 1. Since the match.pd pattern rewrites this to either LE or GT
those are the only two conditions I accept with a rhs of 1 for and then set the appropriate resulting
comparison based on what I understand the spaceship operator to be doing.
This fixes the regression and gets the same codegen as before.
Bootstrapped Regtested on aarch64-none-linux-gnu,
x86_64-pc-linux-gnu and no regressions.
Ok for master?
Thanks,
Tamar
gcc/ChangeLog:
* tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical
codegen.
--- inline copy of patch ---
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index 0e339c46afa29fa97f90d9bc4394370cd9b4b396..1a2f9294e5c3e6a7fd5ade4c21cdedc44e70d911 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -2038,6 +2038,26 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
gimple *orig_use_stmt = use_stmt;
tree orig_use_lhs = NULL_TREE;
int prec = TYPE_PRECISION (TREE_TYPE (phires));
+
+ /* Deal with if match.pd has rewritten the (res & ~1) == 0
+ into res <= 1 and has left a type-cast for signed types. */
+ if (gimple_assign_cast_p (use_stmt))
+ {
+ orig_use_lhs = gimple_assign_lhs (use_stmt);
+ /* Match.pd would have only done this for a signed type,
+ so the conversion must be to an unsigned one. */
+ if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs)))
+ return false;
+ if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs))
+ return false;
+ if (EDGE_COUNT (phi_bb->preds) != 4)
+ return false;
+ if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs)))
+ return false;
+ if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt))
+ return false;
+ }
+
if (is_gimple_assign (use_stmt)
&& gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR
&& TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST
@@ -2099,7 +2119,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
|| !tree_fits_shwi_p (rhs)
|| !IN_RANGE (tree_to_shwi (rhs), -1, 1))
return false;
- if (orig_use_lhs)
+ if (orig_use_lhs && !integer_onep (rhs))
{
if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs))
return false;
@@ -2345,6 +2365,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR;
else if (integer_minus_onep (rhs))
res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR;
+ else if (integer_onep (rhs))
+ res_cmp = GE_EXPR;
else
return false;
break;
@@ -2353,6 +2375,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR;
else if (integer_zerop (rhs))
res_cmp = one_cmp;
+ else if (integer_onep (rhs))
+ res_cmp = one_cmp;
else
return false;
break;
@@ -2360,7 +2384,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
if (integer_zerop (rhs))
res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR;
else if (integer_onep (rhs))
- res_cmp = one_cmp;
+ res_cmp = LE_EXPR;
else
return false;
break;
On Wed, Nov 03, 2021 at 10:56:30AM +0000, Tamar Christina wrote: > The spaceship operator is looking for (res & 1) == res which was previously folded to (res & ~1) == 0. > This is now being folded further to ((unsigned) res) <= 1. Is that match.pd change already in the tree (which commit) or not yet? > --- a/gcc/tree-ssa-phiopt.c > +++ b/gcc/tree-ssa-phiopt.c > @@ -2038,6 +2038,26 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > gimple *orig_use_stmt = use_stmt; > tree orig_use_lhs = NULL_TREE; > int prec = TYPE_PRECISION (TREE_TYPE (phires)); > + > + /* Deal with if match.pd has rewritten the (res & ~1) == 0 > + into res <= 1 and has left a type-cast for signed types. */ The above sentence doesn't make sense gramatically. Either Deal with match.pd rewriting ... and leaving ... or Deal with the case when match.pd ... or something similar? > + if (gimple_assign_cast_p (use_stmt)) > + { > + orig_use_lhs = gimple_assign_lhs (use_stmt); > + /* Match.pd would have only done this for a signed type, > + so the conversion must be to an unsigned one. */ Even at the start of sentence it should be match.pd, the file isn't called Match.pd. > + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) > + return false; > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) > + return false; > + if (EDGE_COUNT (phi_bb->preds) != 4) > + return false; > + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) > + return false; You are testing !TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs)) twice, did you mean to instead test that it is a conversion from signed to unsigned (i.e. test if (TYPE_UNSIGNED (TREE_TYPE (gimple_assign_rhs1 (use_stmt)))) return false; ? Also, shouldn't it also test that both types are integral and have the same precision? > + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) > + return false; > + } > + > if (is_gimple_assign (use_stmt) > && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR > && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST > @@ -2099,7 +2119,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > || !tree_fits_shwi_p (rhs) > || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) > return false; > - if (orig_use_lhs) > + if (orig_use_lhs && !integer_onep (rhs)) This doesn't look safe. orig_use_lhs in this case means either that there was just a cast, or that there was BIT_AND_EXPR, or that were both, and you don't know which one it is. The decision shouldn't be done based on whether rhs is or isn't 1, but on whether there was the BIT_AND or not. > { > if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) > return false; > @@ -2345,6 +2365,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > else if (integer_minus_onep (rhs)) > res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > + else if (integer_onep (rhs)) > + res_cmp = GE_EXPR; And this one should be guarded on either the cast present or the comparison done unsigned (so probably TYPE_UNSIGNED (TREE_TYPE (rhs)) && integer_onep (rhs))? > else > return false; > break; > @@ -2353,6 +2375,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > else if (integer_zerop (rhs)) > res_cmp = one_cmp; > + else if (integer_onep (rhs)) > + res_cmp = one_cmp; > else > return false; > break; Likewise. > @@ -2360,7 +2384,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > if (integer_zerop (rhs)) > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > else if (integer_onep (rhs)) > - res_cmp = one_cmp; > + res_cmp = LE_EXPR; > else > return false; > break; Are you sure? Jakub
> > + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) > > + return false; > > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) > > + return false; > > + if (EDGE_COUNT (phi_bb->preds) != 4) > > + return false; > > + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) > > + return false; > > You are testing !TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs)) twice, did you > mean to instead test that it is a conversion from signed to unsigned (i.e. test > if (TYPE_UNSIGNED (TREE_TYPE (gimple_assign_rhs1 (use_stmt)))) > return false; > ? Also, shouldn't it also test that both types are integral and have the same > precision? > I'm not sure the precision matters since if the conversion resulted in not enough precision such that It influences the compare it would have been optimized out. But I've added the check nonetheless. > > + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) > > + return false; > > + } > > + > > if (is_gimple_assign (use_stmt) > > && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR > > && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST @@ > > -2099,7 +2119,7 @@ spaceship_replacement (basic_block cond_bb, > basic_block middle_bb, > > || !tree_fits_shwi_p (rhs) > > || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) > > return false; > > - if (orig_use_lhs) > > + if (orig_use_lhs && !integer_onep (rhs)) > > This doesn't look safe. orig_use_lhs in this case means either that there was > just a cast, or that there was BIT_AND_EXPR, or that were both, and you > don't know which one it is. > The decision shouldn't be done based on whether rhs is or isn't 1, but on > whether there was the BIT_AND or not. Right in the original patch I guarded this based on whether the conversion was detected or not. I removed it because I thought it was safe enough but have added it back now. > > > { > > if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) > > return false; > > @@ -2345,6 +2365,8 @@ spaceship_replacement (basic_block cond_bb, > basic_block middle_bb, > > res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > > else if (integer_minus_onep (rhs)) > > res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > > + else if (integer_onep (rhs)) > > + res_cmp = GE_EXPR; > > And this one should be guarded on either the cast present or the comparison > done unsigned (so probably TYPE_UNSIGNED (TREE_TYPE (rhs)) && > integer_onep (rhs))? > > > else > > return false; > > break; > > @@ -2353,6 +2375,8 @@ spaceship_replacement (basic_block cond_bb, > basic_block middle_bb, > > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > > else if (integer_zerop (rhs)) > > res_cmp = one_cmp; > > + else if (integer_onep (rhs)) > > + res_cmp = one_cmp; > > else > > return false; > > break; > > Likewise. > > > @@ -2360,7 +2384,7 @@ spaceship_replacement (basic_block cond_bb, > basic_block middle_bb, > > if (integer_zerop (rhs)) > > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > > else if (integer_onep (rhs)) > > - res_cmp = one_cmp; > > + res_cmp = LE_EXPR; > > else > > return false; > > break; > > Are you sure? > No, this part is wrong, was a vim yank failure I should have checked the patch before attaching. Here's an updated patch. Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu and no regressions. Ok for master? Thanks, Tamar gcc/ChangeLog: * tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical codegen. --- inline copy of patch --- diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c index 0e339c46afa29fa97f90d9bc4394370cd9b4b396..e72677087da72c8fa52e159f434c51bdebfc5f2d 100644 --- a/gcc/tree-ssa-phiopt.c +++ b/gcc/tree-ssa-phiopt.c @@ -2038,6 +2038,34 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, gimple *orig_use_stmt = use_stmt; tree orig_use_lhs = NULL_TREE; int prec = TYPE_PRECISION (TREE_TYPE (phires)); + bool is_cast = false; + + /* Deal with the case when match.pd has rewritten the (res & ~1) == 0 + into res <= 1 and has left a type-cast for signed types. */ + if (gimple_assign_cast_p (use_stmt)) + { + orig_use_lhs = gimple_assign_lhs (use_stmt); + /* match.pd would have only done this for a signed type, + so the conversion must be to an unsigned one. */ + tree ty1 = TREE_TYPE (gimple_assign_rhs1 (use_stmt)); + tree ty2 = TREE_TYPE (orig_use_lhs); + + if (TYPE_UNSIGNED (ty1) || !INTEGRAL_TYPE_P (ty1)) + return false; + if (!TYPE_UNSIGNED (ty2) || !INTEGRAL_TYPE_P (ty2)) + return false; + if (TYPE_PRECISION (ty1) != TYPE_PRECISION (ty2)) + return false; + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) + return false; + if (EDGE_COUNT (phi_bb->preds) != 4) + return false; + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) + return false; + + is_cast = true; + } + if (is_gimple_assign (use_stmt) && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST @@ -2099,7 +2127,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, || !tree_fits_shwi_p (rhs) || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) return false; - if (orig_use_lhs) + if (orig_use_lhs && !is_cast) { if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) return false; @@ -2345,6 +2373,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; else if (integer_minus_onep (rhs)) res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; + else if (integer_onep (rhs) && is_cast) + res_cmp = GE_EXPR; else return false; break; @@ -2353,6 +2383,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; else if (integer_zerop (rhs)) res_cmp = one_cmp; + else if (integer_onep (rhs) && is_cast) + res_cmp = LE_EXPR; else return false; break;
On Thu, Nov 04, 2021 at 12:19:34PM +0000, Tamar Christina wrote: > I'm not sure the precision matters since if the conversion resulted in not enough > precision such that It influences the compare it would have been optimized out. You can't really rely on other optimizations being performed. They will usually happen, but might not because such code only materialized short time ago without folding happening in between, or some debug counters or -fno-* disabling some passes, ... > --- a/gcc/tree-ssa-phiopt.c > +++ b/gcc/tree-ssa-phiopt.c > @@ -2038,6 +2038,34 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > gimple *orig_use_stmt = use_stmt; > tree orig_use_lhs = NULL_TREE; > int prec = TYPE_PRECISION (TREE_TYPE (phires)); > + bool is_cast = false; > + > + /* Deal with the case when match.pd has rewritten the (res & ~1) == 0 > + into res <= 1 and has left a type-cast for signed types. */ > + if (gimple_assign_cast_p (use_stmt)) > + { > + orig_use_lhs = gimple_assign_lhs (use_stmt); > + /* match.pd would have only done this for a signed type, > + so the conversion must be to an unsigned one. */ > + tree ty1 = TREE_TYPE (gimple_assign_rhs1 (use_stmt)); > + tree ty2 = TREE_TYPE (orig_use_lhs); gimple_assign_rhs1 (use_stmt) is I think guaranteed to be phires here. And that has some of this checked already at the start of the function: if (!INTEGRAL_TYPE_P (TREE_TYPE (phires)) || TYPE_UNSIGNED (TREE_TYPE (phires)) > + > + if (TYPE_UNSIGNED (ty1) || !INTEGRAL_TYPE_P (ty1)) > + return false; So I think the above two lines are redundant. > + if (!TYPE_UNSIGNED (ty2) || !INTEGRAL_TYPE_P (ty2)) > + return false; > + if (TYPE_PRECISION (ty1) != TYPE_PRECISION (ty2)) > + return false; > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) > + return false; > + if (EDGE_COUNT (phi_bb->preds) != 4) > + return false; > + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) > + return false; > + > + is_cast = true; > + } > + > if (is_gimple_assign (use_stmt) I'd feel much safer if this was else if rather than if. The reason for the patch is that (res & ~1) == 0 is optimized into (unsigned) res <= 1, right, so it can be either this or that and you don't need both. If you want to also handle both, that would mean figuring all the details even for that case, handling of debug stmts etc. > && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR > && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST > @@ -2099,7 +2127,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > || !tree_fits_shwi_p (rhs) > || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) > return false; > - if (orig_use_lhs) > + if (orig_use_lhs && !is_cast) Because otherwise it is unclear what the above means, the intent is that the if handles the case where BIT_AND_EXPR is present, but with both cast to unsigned and BIT_AND_EXPR present it acts differently. > @@ -2345,6 +2373,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > else if (integer_minus_onep (rhs)) > res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > + else if (integer_onep (rhs) && is_cast) > + res_cmp = GE_EXPR; > else > return false; > break; > @@ -2353,6 +2383,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > else if (integer_zerop (rhs)) > res_cmp = one_cmp; > + else if (integer_onep (rhs) && is_cast) > + res_cmp = LE_EXPR; > else > return false; > break; I'm afraid this is still wrong. Because is_cast which implies that the comparison is done in unsigned type rather than signed type which is otherwise ensured changes everything the code assumes. While maybe EQ_EXPR and NE_EXPR will work the same whether it is unsigned or signed comparison, the other comparisons certainly will not. So, my preference would be instead of doing these 2 hunks handle the is_cast case early, right before if (orig_use_lhs) above. Something like: if (is_cast) { if (TREE_CODE (rhs) != INTEGER_CST) return false; /* As for -ffast-math we assume the 2 return to be impossible, canonicalize (unsigned) res <= 1U or (unsigned) res < 2U into res >= 0 and (unsigned) res > 1U or (unsigned) res >= 2U as res < 0. */ switch (cmp) { case LE_EXPR: if (!integer_onep (rhs)) return false; cmp = GE_EXPR; break; case LT_EXPR: if (wi::ne_p (wi::to_widest (rhs), 2)) return false; cmp = GE_EXPR; break; case GT_EXPR: if (!integer_onep (rhs)) return false; cmp = LT_EXPR; break; case GE_EXPR: if (wi::ne_p (wi::to_widest (rhs), 2)) return false; cmp = LT_EXPR; break; default: return false; } rhs = build_zero_cst (TREE_TYPE (phires)); } else if (orig_use_lhs) { ... Similarly to the BIT_AND_EXPR case the above transforms the is_cast case into a signed comparison that the later code knows how to handle. There is another problem though. If there are debug stmts, the code later on does: /* If there are debug uses, emit something like: # DEBUG D#1 => i_2(D) > j_3(D) ? 1 : -1 # DEBUG D#2 => i_2(D) == j_3(D) ? 0 : D#1 where > stands for the comparison that yielded 1 and replace debug uses of phi result with that D#2. Ignore the value of 2, because if NaNs aren't expected, all floating point numbers should be comparable. */ ... replace_uses_by (phires, temp2); if (orig_use_lhs) replace_uses_by (orig_use_lhs, temp2); For the is_cast case this creates invalid IL, because the uses of orig_use_lhs if any expect an unsigned value, but temp2 is signed. Perhaps when one uses <compare> stuff this will never happen, but there is nothing that prevents a user to write his own code so that spaceship_replacement actually triggers on it (i.e. results in the same IL) and has debug info uses on it. And we can't e.g. punt if there are debug stmts and not otherwise because that would result in -fcompare-debug differences. So, I think we need: bool has_debug_uses = false; + bool has_cast_debug_uses = false; ... - if (!has_debug_uses) + if (!has_debug_uses || is_cast) FOR_EACH_IMM_USE_FAST (use_p, iter, orig_use_lhs) { gimple *use_stmt = USE_STMT (use_p); gcc_assert (is_gimple_debug (use_stmt)); has_debug_uses = true; + if (is_cast) + has_cast_debug_uses = true; } and later if (has_cast_debug_uses) create another debug temporary temp3 with TREE_TYPE set to TREE_TYPE (orig_use_lhs), the expression (that_uns_type) temp2 and use that temp3 instead of temp2 in replace_uses_by (orig_use_lhs, temp2); Jakub
> -----Original Message----- > From: Jakub Jelinek <jakub@redhat.com> > Sent: Thursday, November 4, 2021 4:11 PM > To: Tamar Christina <Tamar.Christina@arm.com> > Cc: Jonathan Wakely <jwakely@redhat.com>; Richard Biener > <rguenther@suse.de>; gcc-patches@gcc.gnu.org; nd <nd@arm.com> > Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns > on signed values > > On Thu, Nov 04, 2021 at 12:19:34PM +0000, Tamar Christina wrote: > > I'm not sure the precision matters since if the conversion resulted in > > not enough precision such that It influences the compare it would have > been optimized out. > > You can't really rely on other optimizations being performed. They will > usually happen, but might not because such code only materialized short > time ago without folding happening in between, or some debug counters or - > fno-* disabling some passes, ... Fair point, I have separated out the logic as you requested and added the debug fix. Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu and no regressions. Ok for master? Thanks, Tamar gcc/ChangeLog: * tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical codegen. --- inline copy of patch --- diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c index 0e339c46afa29fa97f90d9bc4394370cd9b4b396..3ad5b23885a37eec0beff229e2a96e86658b2d1a 100644 --- a/gcc/tree-ssa-phiopt.c +++ b/gcc/tree-ssa-phiopt.c @@ -2038,11 +2038,36 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, gimple *orig_use_stmt = use_stmt; tree orig_use_lhs = NULL_TREE; int prec = TYPE_PRECISION (TREE_TYPE (phires)); - if (is_gimple_assign (use_stmt) - && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR - && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST - && (wi::to_wide (gimple_assign_rhs2 (use_stmt)) - == wi::shifted_mask (1, prec - 1, false, prec))) + bool is_cast = false; + + /* Deal with the case when match.pd has rewritten the (res & ~1) == 0 + into res <= 1 and has left a type-cast for signed types. */ + if (gimple_assign_cast_p (use_stmt)) + { + orig_use_lhs = gimple_assign_lhs (use_stmt); + /* match.pd would have only done this for a signed type, + so the conversion must be to an unsigned one. */ + tree ty1 = TREE_TYPE (gimple_assign_rhs1 (use_stmt)); + tree ty2 = TREE_TYPE (orig_use_lhs); + + if (!TYPE_UNSIGNED (ty2) || !INTEGRAL_TYPE_P (ty2)) + return false; + if (TYPE_PRECISION (ty1) != TYPE_PRECISION (ty2)) + return false; + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) + return false; + if (EDGE_COUNT (phi_bb->preds) != 4) + return false; + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) + return false; + + is_cast = true; + } + else if (is_gimple_assign (use_stmt) + && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR + && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST + && (wi::to_wide (gimple_assign_rhs2 (use_stmt)) + == wi::shifted_mask (1, prec - 1, false, prec))) { /* For partial_ordering result operator>= with unspec as second argument is (res & 1) == res, folded by match.pd into @@ -2099,7 +2124,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, || !tree_fits_shwi_p (rhs) || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) return false; - if (orig_use_lhs) + if (orig_use_lhs && !is_cast) { if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) return false; @@ -2310,62 +2335,101 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, one_cmp = GT_EXPR; enum tree_code res_cmp; - switch (cmp) + + if (is_cast) { - case EQ_EXPR: - if (integer_zerop (rhs)) - res_cmp = EQ_EXPR; - else if (integer_minus_onep (rhs)) - res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; - else if (integer_onep (rhs)) - res_cmp = one_cmp; - else + if (TREE_CODE (rhs) != INTEGER_CST) return false; - break; - case NE_EXPR: - if (integer_zerop (rhs)) - res_cmp = NE_EXPR; - else if (integer_minus_onep (rhs)) - res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; - else if (integer_onep (rhs)) - res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; - else - return false; - break; - case LT_EXPR: - if (integer_onep (rhs)) - res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; - else if (integer_zerop (rhs)) - res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; - else - return false; - break; - case LE_EXPR: - if (integer_zerop (rhs)) - res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; - else if (integer_minus_onep (rhs)) - res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; - else - return false; - break; - case GT_EXPR: - if (integer_minus_onep (rhs)) - res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; - else if (integer_zerop (rhs)) - res_cmp = one_cmp; - else - return false; - break; - case GE_EXPR: - if (integer_zerop (rhs)) - res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; - else if (integer_onep (rhs)) - res_cmp = one_cmp; - else - return false; - break; - default: - gcc_unreachable (); + /* As for -ffast-math we assume the 2 return to be + impossible, canonicalize (unsigned) res <= 1U or + (unsigned) res < 2U into res >= 0 and (unsigned) res > 1U + or (unsigned) res >= 2U as res < 0. */ + switch (cmp) + { + case LE_EXPR: + if (!integer_onep (rhs)) + return false; + res_cmp = GE_EXPR; + break; + case LT_EXPR: + if (wi::ne_p (wi::to_widest (rhs), 2)) + return false; + res_cmp = GE_EXPR; + break; + case GT_EXPR: + if (!integer_onep (rhs)) + return false; + res_cmp = LT_EXPR; + break; + case GE_EXPR: + if (wi::ne_p (wi::to_widest (rhs), 2)) + return false; + res_cmp = LT_EXPR; + break; + default: + return false; + } + rhs = build_zero_cst (TREE_TYPE (phires)); + } + else + { + switch (cmp) + { + case EQ_EXPR: + if (integer_zerop (rhs)) + res_cmp = EQ_EXPR; + else if (integer_minus_onep (rhs)) + res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; + else if (integer_onep (rhs)) + res_cmp = one_cmp; + else + return false; + break; + case NE_EXPR: + if (integer_zerop (rhs)) + res_cmp = NE_EXPR; + else if (integer_minus_onep (rhs)) + res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; + else if (integer_onep (rhs)) + res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; + else + return false; + break; + case LT_EXPR: + if (integer_onep (rhs)) + res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; + else if (integer_zerop (rhs)) + res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; + else + return false; + break; + case LE_EXPR: + if (integer_zerop (rhs)) + res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; + else if (integer_minus_onep (rhs)) + res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; + else + return false; + break; + case GT_EXPR: + if (integer_minus_onep (rhs)) + res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; + else if (integer_zerop (rhs)) + res_cmp = one_cmp; + else + return false; + break; + case GE_EXPR: + if (integer_zerop (rhs)) + res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; + else if (integer_onep (rhs)) + res_cmp = one_cmp; + else + return false; + break; + default: + gcc_unreachable (); + } } if (gimple_code (use_stmt) == GIMPLE_COND) @@ -2394,6 +2458,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, use_operand_p use_p; imm_use_iterator iter; bool has_debug_uses = false; + bool has_cast_debug_uses = false; FOR_EACH_IMM_USE_FAST (use_p, iter, phires) { gimple *use_stmt = USE_STMT (use_p); @@ -2405,12 +2470,14 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, } if (orig_use_lhs) { - if (!has_debug_uses) + if (!has_debug_uses || is_cast) FOR_EACH_IMM_USE_FAST (use_p, iter, orig_use_lhs) { gimple *use_stmt = USE_STMT (use_p); gcc_assert (is_gimple_debug (use_stmt)); has_debug_uses = true; + if (is_cast) + has_cast_debug_uses = true; } gimple_stmt_iterator gsi = gsi_for_stmt (orig_use_stmt); tree zero = build_zero_cst (TREE_TYPE (orig_use_lhs)); @@ -2448,7 +2515,23 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, gsi_insert_before (&gsi, g, GSI_SAME_STMT); replace_uses_by (phires, temp2); if (orig_use_lhs) - replace_uses_by (orig_use_lhs, temp2); + { + if (has_cast_debug_uses) + { + tree temp3 = make_node (DEBUG_EXPR_DECL); + DECL_ARTIFICIAL (temp3) = 1; + TREE_TYPE (temp3) = TREE_TYPE (orig_use_lhs); + SET_DECL_MODE (temp3, TYPE_MODE (type)); + t = build2 (EQ_EXPR, boolean_type_node, lhs1, rhs2); + t = build3 (COND_EXPR, type, t, build_zero_cst (type), + temp1); + g = gimple_build_debug_bind (temp3, t, phi); + gsi_insert_before (&gsi, g, GSI_SAME_STMT); + replace_uses_by (orig_use_lhs, temp3); + } + else + replace_uses_by (orig_use_lhs, temp2); + } } }
Ping > -----Original Message----- > From: Tamar Christina > Sent: Friday, November 12, 2021 7:31 AM > To: Jakub Jelinek <jakub@redhat.com> > Cc: Jonathan Wakely <jwakely@redhat.com>; Richard Biener > <rguenther@suse.de>; gcc-patches@gcc.gnu.org; nd <nd@arm.com> > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns > on signed values > > > > > -----Original Message----- > > From: Jakub Jelinek <jakub@redhat.com> > > Sent: Thursday, November 4, 2021 4:11 PM > > To: Tamar Christina <Tamar.Christina@arm.com> > > Cc: Jonathan Wakely <jwakely@redhat.com>; Richard Biener > > <rguenther@suse.de>; gcc-patches@gcc.gnu.org; nd <nd@arm.com> > > Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear > > patterns on signed values > > > > On Thu, Nov 04, 2021 at 12:19:34PM +0000, Tamar Christina wrote: > > > I'm not sure the precision matters since if the conversion resulted > > > in not enough precision such that It influences the compare it would > > > have > > been optimized out. > > > > You can't really rely on other optimizations being performed. They > > will usually happen, but might not because such code only materialized > > short time ago without folding happening in between, or some debug > > counters or - > > fno-* disabling some passes, ... > > Fair point, I have separated out the logic as you requested and added the > debug fix. > > Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu > and no regressions. > > Ok for master? > > Thanks, > Tamar > > gcc/ChangeLog: > > * tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical > codegen. > > --- inline copy of patch --- > > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c index > 0e339c46afa29fa97f90d9bc4394370cd9b4b396..3ad5b23885a37eec0beff229e2 > a96e86658b2d1a 100644 > --- a/gcc/tree-ssa-phiopt.c > +++ b/gcc/tree-ssa-phiopt.c > @@ -2038,11 +2038,36 @@ spaceship_replacement (basic_block cond_bb, > basic_block middle_bb, > gimple *orig_use_stmt = use_stmt; > tree orig_use_lhs = NULL_TREE; > int prec = TYPE_PRECISION (TREE_TYPE (phires)); > - if (is_gimple_assign (use_stmt) > - && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR > - && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST > - && (wi::to_wide (gimple_assign_rhs2 (use_stmt)) > - == wi::shifted_mask (1, prec - 1, false, prec))) > + bool is_cast = false; > + > + /* Deal with the case when match.pd has rewritten the (res & ~1) == 0 > + into res <= 1 and has left a type-cast for signed types. */ > + if (gimple_assign_cast_p (use_stmt)) > + { > + orig_use_lhs = gimple_assign_lhs (use_stmt); > + /* match.pd would have only done this for a signed type, > + so the conversion must be to an unsigned one. */ > + tree ty1 = TREE_TYPE (gimple_assign_rhs1 (use_stmt)); > + tree ty2 = TREE_TYPE (orig_use_lhs); > + > + if (!TYPE_UNSIGNED (ty2) || !INTEGRAL_TYPE_P (ty2)) > + return false; > + if (TYPE_PRECISION (ty1) != TYPE_PRECISION (ty2)) > + return false; > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) > + return false; > + if (EDGE_COUNT (phi_bb->preds) != 4) > + return false; > + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) > + return false; > + > + is_cast = true; > + } > + else if (is_gimple_assign (use_stmt) > + && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR > + && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST > + && (wi::to_wide (gimple_assign_rhs2 (use_stmt)) > + == wi::shifted_mask (1, prec - 1, false, prec))) > { > /* For partial_ordering result operator>= with unspec as second > argument is (res & 1) == res, folded by match.pd into @@ -2099,7 > +2124,7 @@ spaceship_replacement (basic_block cond_bb, basic_block > middle_bb, > || !tree_fits_shwi_p (rhs) > || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) > return false; > - if (orig_use_lhs) > + if (orig_use_lhs && !is_cast) > { > if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) > return false; > @@ -2310,62 +2335,101 @@ spaceship_replacement (basic_block cond_bb, > basic_block middle_bb, > one_cmp = GT_EXPR; > > enum tree_code res_cmp; > - switch (cmp) > + > + if (is_cast) > { > - case EQ_EXPR: > - if (integer_zerop (rhs)) > - res_cmp = EQ_EXPR; > - else if (integer_minus_onep (rhs)) > - res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > - else if (integer_onep (rhs)) > - res_cmp = one_cmp; > - else > + if (TREE_CODE (rhs) != INTEGER_CST) > return false; > - break; > - case NE_EXPR: > - if (integer_zerop (rhs)) > - res_cmp = NE_EXPR; > - else if (integer_minus_onep (rhs)) > - res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > - else if (integer_onep (rhs)) > - res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > - else > - return false; > - break; > - case LT_EXPR: > - if (integer_onep (rhs)) > - res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > - else if (integer_zerop (rhs)) > - res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > - else > - return false; > - break; > - case LE_EXPR: > - if (integer_zerop (rhs)) > - res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > - else if (integer_minus_onep (rhs)) > - res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > - else > - return false; > - break; > - case GT_EXPR: > - if (integer_minus_onep (rhs)) > - res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > - else if (integer_zerop (rhs)) > - res_cmp = one_cmp; > - else > - return false; > - break; > - case GE_EXPR: > - if (integer_zerop (rhs)) > - res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > - else if (integer_onep (rhs)) > - res_cmp = one_cmp; > - else > - return false; > - break; > - default: > - gcc_unreachable (); > + /* As for -ffast-math we assume the 2 return to be > + impossible, canonicalize (unsigned) res <= 1U or > + (unsigned) res < 2U into res >= 0 and (unsigned) res > 1U > + or (unsigned) res >= 2U as res < 0. */ > + switch (cmp) > + { > + case LE_EXPR: > + if (!integer_onep (rhs)) > + return false; > + res_cmp = GE_EXPR; > + break; > + case LT_EXPR: > + if (wi::ne_p (wi::to_widest (rhs), 2)) > + return false; > + res_cmp = GE_EXPR; > + break; > + case GT_EXPR: > + if (!integer_onep (rhs)) > + return false; > + res_cmp = LT_EXPR; > + break; > + case GE_EXPR: > + if (wi::ne_p (wi::to_widest (rhs), 2)) > + return false; > + res_cmp = LT_EXPR; > + break; > + default: > + return false; > + } > + rhs = build_zero_cst (TREE_TYPE (phires)); > + } > + else > + { > + switch (cmp) > + { > + case EQ_EXPR: > + if (integer_zerop (rhs)) > + res_cmp = EQ_EXPR; > + else if (integer_minus_onep (rhs)) > + res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > + else if (integer_onep (rhs)) > + res_cmp = one_cmp; > + else > + return false; > + break; > + case NE_EXPR: > + if (integer_zerop (rhs)) > + res_cmp = NE_EXPR; > + else if (integer_minus_onep (rhs)) > + res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > + else if (integer_onep (rhs)) > + res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > + else > + return false; > + break; > + case LT_EXPR: > + if (integer_onep (rhs)) > + res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > + else if (integer_zerop (rhs)) > + res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > + else > + return false; > + break; > + case LE_EXPR: > + if (integer_zerop (rhs)) > + res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > + else if (integer_minus_onep (rhs)) > + res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > + else > + return false; > + break; > + case GT_EXPR: > + if (integer_minus_onep (rhs)) > + res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > + else if (integer_zerop (rhs)) > + res_cmp = one_cmp; > + else > + return false; > + break; > + case GE_EXPR: > + if (integer_zerop (rhs)) > + res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > + else if (integer_onep (rhs)) > + res_cmp = one_cmp; > + else > + return false; > + break; > + default: > + gcc_unreachable (); > + } > } > > if (gimple_code (use_stmt) == GIMPLE_COND) @@ -2394,6 +2458,7 @@ > spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > use_operand_p use_p; > imm_use_iterator iter; > bool has_debug_uses = false; > + bool has_cast_debug_uses = false; > FOR_EACH_IMM_USE_FAST (use_p, iter, phires) > { > gimple *use_stmt = USE_STMT (use_p); @@ -2405,12 +2470,14 @@ > spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > } > if (orig_use_lhs) > { > - if (!has_debug_uses) > + if (!has_debug_uses || is_cast) > FOR_EACH_IMM_USE_FAST (use_p, iter, orig_use_lhs) > { > gimple *use_stmt = USE_STMT (use_p); > gcc_assert (is_gimple_debug (use_stmt)); > has_debug_uses = true; > + if (is_cast) > + has_cast_debug_uses = true; > } > gimple_stmt_iterator gsi = gsi_for_stmt (orig_use_stmt); > tree zero = build_zero_cst (TREE_TYPE (orig_use_lhs)); @@ -2448,7 > +2515,23 @@ spaceship_replacement (basic_block cond_bb, basic_block > middle_bb, > gsi_insert_before (&gsi, g, GSI_SAME_STMT); > replace_uses_by (phires, temp2); > if (orig_use_lhs) > - replace_uses_by (orig_use_lhs, temp2); > + { > + if (has_cast_debug_uses) > + { > + tree temp3 = make_node (DEBUG_EXPR_DECL); > + DECL_ARTIFICIAL (temp3) = 1; > + TREE_TYPE (temp3) = TREE_TYPE (orig_use_lhs); > + SET_DECL_MODE (temp3, TYPE_MODE (type)); > + t = build2 (EQ_EXPR, boolean_type_node, lhs1, rhs2); > + t = build3 (COND_EXPR, type, t, build_zero_cst (type), > + temp1); > + g = gimple_build_debug_bind (temp3, t, phi); > + gsi_insert_before (&gsi, g, GSI_SAME_STMT); > + replace_uses_by (orig_use_lhs, temp3); > + } > + else > + replace_uses_by (orig_use_lhs, temp2); > + } > } > }
On Fri, Nov 12, 2021 at 07:30:35AM +0000, Tamar Christina via Gcc-patches wrote: > @@ -2099,7 +2124,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > || !tree_fits_shwi_p (rhs) > || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) > return false; > - if (orig_use_lhs) > + if (orig_use_lhs && !is_cast) > { > if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) > return false; I actually meant that you'd do the if (is_cast) handling right above the if (orig_use_lhs), i.e. if (is_cast) { if (TREE_CODE (rhs) != INTEGER_CST) return false; /* As for -ffast-math we assume the 2 return to be impossible, canonicalize (unsigned) res <= 1U or (unsigned) res < 2U into res >= 0 and (unsigned) res > 1U or (unsigned) res >= 2U as res < 0. */ switch (cmp) { case LE_EXPR: if (!integer_onep (rhs)) return false; cmp = GE_EXPR; break; case LT_EXPR: if (wi::ne_p (wi::to_widest (rhs), 2)) return false; cmp = GE_EXPR; break; case GT_EXPR: if (!integer_onep (rhs)) return false; cmp = LT_EXPR; break; case GE_EXPR: if (wi::ne_p (wi::to_widest (rhs), 2)) return false; cmp = LT_EXPR; break; default: return false; } rhs = build_zero_cst (TREE_TYPE (phires)); } else if (orig_use_lhs) ... and keep the code in the following hunk untouched. Similarly to how for the BIT_AND_EXPR if (orig_use_lhs), it virtually undoes the match.pd optimization. Because in the place you've placed it you're totally ignoring one_cmp, and I'm pretty sure that is the wrong thing. one_cmp is computed as: /* lhs1 one_cmp rhs1 results in phires of 1. */ enum tree_code one_cmp; if ((cmp1 == LT_EXPR || cmp1 == LE_EXPR) ^ (!integer_onep ((e1->flags & EDGE_TRUE_VALUE) ? arg1 : arg0))) one_cmp = LT_EXPR; else one_cmp = GT_EXPR; and it is something unrelated to what actual comparison is done or virtually done on the phires. > @@ -2310,62 +2335,101 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > one_cmp = GT_EXPR; > > enum tree_code res_cmp; > - switch (cmp) > + > + if (is_cast) > { > - case EQ_EXPR: > - if (integer_zerop (rhs)) > - res_cmp = EQ_EXPR; > - else if (integer_minus_onep (rhs)) > - res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > - else if (integer_onep (rhs)) > - res_cmp = one_cmp; > - else > + if (TREE_CODE (rhs) != INTEGER_CST) > return false; > - break; > - case NE_EXPR: > - if (integer_zerop (rhs)) > - res_cmp = NE_EXPR; > - else if (integer_minus_onep (rhs)) > - res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > - else if (integer_onep (rhs)) > - res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > - else > - return false; > - break; > - case LT_EXPR: > - if (integer_onep (rhs)) > - res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > - else if (integer_zerop (rhs)) > - res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > - else > - return false; > - break; > - case LE_EXPR: > - if (integer_zerop (rhs)) > - res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > - else if (integer_minus_onep (rhs)) > - res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > - else > - return false; > - break; > - case GT_EXPR: > - if (integer_minus_onep (rhs)) > - res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > - else if (integer_zerop (rhs)) > - res_cmp = one_cmp; > - else > - return false; > - break; > - case GE_EXPR: > - if (integer_zerop (rhs)) > - res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > - else if (integer_onep (rhs)) > - res_cmp = one_cmp; > - else > - return false; > - break; > - default: > - gcc_unreachable (); > + /* As for -ffast-math we assume the 2 return to be > + impossible, canonicalize (unsigned) res <= 1U or > + (unsigned) res < 2U into res >= 0 and (unsigned) res > 1U > + or (unsigned) res >= 2U as res < 0. */ > + switch (cmp) > + { > + case LE_EXPR: > + if (!integer_onep (rhs)) > + return false; > + res_cmp = GE_EXPR; > + break; > + case LT_EXPR: > + if (wi::ne_p (wi::to_widest (rhs), 2)) > + return false; > + res_cmp = GE_EXPR; > + break; > + case GT_EXPR: > + if (!integer_onep (rhs)) > + return false; > + res_cmp = LT_EXPR; > + break; > + case GE_EXPR: > + if (wi::ne_p (wi::to_widest (rhs), 2)) > + return false; > + res_cmp = LT_EXPR; > + break; > + default: > + return false; > + } > + rhs = build_zero_cst (TREE_TYPE (phires)); > + } > + else > + { > + switch (cmp) > + { > + case EQ_EXPR: > + if (integer_zerop (rhs)) > + res_cmp = EQ_EXPR; > + else if (integer_minus_onep (rhs)) > + res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > + else if (integer_onep (rhs)) > + res_cmp = one_cmp; > + else > + return false; > + break; > + case NE_EXPR: > + if (integer_zerop (rhs)) > + res_cmp = NE_EXPR; > + else if (integer_minus_onep (rhs)) > + res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > + else if (integer_onep (rhs)) > + res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > + else > + return false; > + break; > + case LT_EXPR: > + if (integer_onep (rhs)) > + res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > + else if (integer_zerop (rhs)) > + res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > + else > + return false; > + break; > + case LE_EXPR: > + if (integer_zerop (rhs)) > + res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > + else if (integer_minus_onep (rhs)) > + res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > + else > + return false; > + break; > + case GT_EXPR: > + if (integer_minus_onep (rhs)) > + res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > + else if (integer_zerop (rhs)) > + res_cmp = one_cmp; > + else > + return false; > + break; > + case GE_EXPR: > + if (integer_zerop (rhs)) > + res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > + else if (integer_onep (rhs)) > + res_cmp = one_cmp; > + else > + return false; > + break; > + default: > + gcc_unreachable (); > + } > } > > if (gimple_code (use_stmt) == GIMPLE_COND) > @@ -2405,12 +2470,14 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > } > if (orig_use_lhs) > { > - if (!has_debug_uses) > + if (!has_debug_uses || is_cast) > FOR_EACH_IMM_USE_FAST (use_p, iter, orig_use_lhs) > { > gimple *use_stmt = USE_STMT (use_p); > gcc_assert (is_gimple_debug (use_stmt)); > has_debug_uses = true; > + if (is_cast) > + has_cast_debug_uses = true; > } > gimple_stmt_iterator gsi = gsi_for_stmt (orig_use_stmt); > tree zero = build_zero_cst (TREE_TYPE (orig_use_lhs)); > @@ -2448,7 +2515,23 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > gsi_insert_before (&gsi, g, GSI_SAME_STMT); > replace_uses_by (phires, temp2); > if (orig_use_lhs) > - replace_uses_by (orig_use_lhs, temp2); > + { > + if (has_cast_debug_uses) > + { > + tree temp3 = make_node (DEBUG_EXPR_DECL); > + DECL_ARTIFICIAL (temp3) = 1; > + TREE_TYPE (temp3) = TREE_TYPE (orig_use_lhs); > + SET_DECL_MODE (temp3, TYPE_MODE (type)); > + t = build2 (EQ_EXPR, boolean_type_node, lhs1, rhs2); > + t = build3 (COND_EXPR, type, t, build_zero_cst (type), > + temp1); This will create a debug stmt with correct type on lhs, but incorrect on the rhs (type rather than TREE_TYPE (orig_use_lhs). You should instead of the above 3 lines do: t = fold_convert (TREE_TYPE (temp3), temp2); Otherwise LGTM. Jakub
diff --git a/gcc/match.pd b/gcc/match.pd index 9532cae582e152cae6e22fcce95a9744a844e3c2..d26e498447fc25a327a42cc6a119c6153d09ba03 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -4945,7 +4945,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) icmp (le le gt le gt) (simplify (cmp (bit_and:c@2 @0 cst@1) integer_zerop) - (with { tree csts = bitmask_inv_cst_vector_p (@1); } + (if (canonicalize_math_after_vectorization_p ()) + (with { tree csts = bitmask_inv_cst_vector_p (@1); } (switch (if (csts && TYPE_UNSIGNED (TREE_TYPE (@1)) && (VECTOR_TYPE_P (TREE_TYPE (@1)) || single_use (@2))) @@ -4954,7 +4955,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) && (cmp == EQ_EXPR || cmp == NE_EXPR) && (VECTOR_TYPE_P (TREE_TYPE (@1)) || single_use (@2))) (with { tree utype = unsigned_type_for (TREE_TYPE (@1)); } - (icmp (convert:utype @0) { csts; })))))))) + (icmp (convert:utype @0) { csts; }))))))))) /* -A CMP -B -> B CMP A. */ (for cmp (tcc_comparison)
Hi All, During testing after rebasing to commit I noticed a failing testcase with the bitmask compare patch. Consider the following C++ testcase: #include <compare> #define A __attribute__((noipa)) A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0; } This turns into a comparison against chars, on systems where chars are signed the pattern inserts an unsigned convert such that it's able to do the transformation. i.e.: # RANGE [-1, 2] # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)> # RANGE ~[3, 254] _11 = (unsigned char) c$_M_value_22; _19 = _11 <= 1; # .MEM_24 = VDEF <.MEM_6(D)> D.10434 ={v} {CLOBBER}; # .MEM_14 = VDEF <.MEM_24> D.10407 ={v} {CLOBBER}; # VUSE <.MEM_14> return _19; instead of: # RANGE [-1, 2] # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)> # RANGE [-2, 2] _3 = c$_M_value_5 & -2; _19 = _3 == 0; # .MEM_24 = VDEF <.MEM_6(D)> D.10440 ={v} {CLOBBER}; # .MEM_14 = VDEF <.MEM_24> D.10413 ={v} {CLOBBER}; # VUSE <.MEM_14> return _19; This causes much worse codegen under -ffast-math due to phiops no longer recognizing the pattern. It turns out that phiopts spaceship_replacement is looking for the exact form that was just changed. Trying to get it to recognize the new form is not trivial as the transformation doesn't look to work when the thing it's pointing to is itself a phi-node. Because of these issues this change delays the replacements until after loop opts. This fixes the failing C++ testcase. Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu and no regressions. Ok for master? Thanks, Tamar gcc/ChangeLog: * match.pd: Delay bitmask compare pattern till after loop opts. --- inline copy of patch -- diff --git a/gcc/match.pd b/gcc/match.pd index 9532cae582e152cae6e22fcce95a9744a844e3c2..d26e498447fc25a327a42cc6a119c6153d09ba03 100644 --