> -----Original Message-----
> From: Richard Biener <rguent...@suse.de>
> Sent: Tuesday, October 26, 2021 9:46 AM
> To: Tamar Christina <tamar.christ...@arm.com>
> Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <ja...@redhat.com>; nd
> <n...@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 <rguent...@suse.de>
> > > Sent: Tuesday, October 26, 2021 9:26 AM
> > > To: Tamar Christina <tamar.christ...@arm.com>
> > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <ja...@redhat.com>; nd
> > > <n...@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 <rguent...@suse.de>
> > > > > Sent: Friday, October 15, 2021 12:31 PM
> > > > > To: Tamar Christina <tamar.christ...@arm.com>
> > > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <ja...@redhat.com>;
> > > > > nd <n...@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 <rguent...@suse.de>
> > > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409
> > > Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
> >
>
> --
> Richard Biener <rguent...@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409
> Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
