From: Tamar Christina <Tamar.Christina@arm.com>
To: Richard Biener <rguenther@suse.de>
Cc: "gcc-patches@gcc.gnu.org" <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
Date: Mon, 25 Oct 2021 14:26:07 +0000 [thread overview]
Message-ID: <VI1PR08MB5325F37BC71A4A26E080FC22FF839@VI1PR08MB5325.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <qo12s6sr-59s-72q-5r8q-3nn2643nn78p@fhfr.qr>
[-- Attachment #1: Type: text/plain, Size: 6217 bytes --]
> -----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;
[-- Attachment #2: rb14938.patch --]
[-- Type: application/octet-stream, Size: 2866 bytes --]
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;
next prev parent reply other threads:[~2021-10-25 14:26 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-15 11:08 Tamar Christina
2021-10-15 11:30 ` Richard Biener
2021-10-25 14:26 ` Tamar Christina [this message]
2021-10-26 8:26 ` Richard Biener
2021-10-26 8:35 ` Tamar Christina
2021-10-26 8:45 ` Richard Biener
2021-10-26 12:10 ` Tamar Christina
2021-10-26 13:13 ` Richard Biener
2021-10-26 13:20 ` Jakub Jelinek
2021-10-26 13:21 ` Richard Biener
2021-10-26 13:36 ` Jakub Jelinek
2021-10-26 13:38 ` Richard Biener
2021-10-26 19:35 ` Jonathan Wakely
2021-10-26 19:39 ` Jakub Jelinek
2021-10-26 19:50 ` Jonathan Wakely
2021-11-03 10:56 ` Tamar Christina
2021-11-03 14:20 ` Jakub Jelinek
2021-11-04 12:19 ` Tamar Christina
2021-11-04 15:10 ` Jakub Jelinek
2021-11-12 7:30 ` Tamar Christina
2021-11-19 8:52 ` Tamar Christina
2021-11-19 11:19 ` Jakub Jelinek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=VI1PR08MB5325F37BC71A4A26E080FC22FF839@VI1PR08MB5325.eurprd08.prod.outlook.com \
--to=tamar.christina@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=nd@arm.com \
--cc=rguenther@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).