* Re: [PATCH] phiopt: Fix up conditional_replacement [PR99305]
@ 2021-03-10 16:25 David Edelsohn
0 siblings, 0 replies; 4+ messages in thread
From: David Edelsohn @ 2021-03-10 16:25 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: GCC Patches
The new pr99305.C testcase is failing on Power.
FAIL: g++.dg/opt/pr99305.C -std=gnu++14 scan-tree-dump-times
optimized " = \\\\(unsigned char\\\\) c_[0-9]*\\\\(D\\\\);" 3
FAIL: g++.dg/opt/pr99305.C -std=gnu++17 scan-tree-dump-times
optimized " = \\\\(unsigned char\\\\) c_[0-9]*\\\\(D\\\\);" 3
FAIL: g++.dg/opt/pr99305.C -std=gnu++2a scan-tree-dump-times
optimized " = \\\\(unsigned char\\\\) c_[0-9]*\\\\(D\\\\);" 3
FAIL: g++.dg/opt/pr99305.C -std=gnu++98 scan-tree-dump-times
optimized " = \\\\(unsigned char\\\\) c_[0-9]*\\\\(D\\\\);" 3
Signed char versus unsigned char defaults?
Thanks, David
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] phiopt: Fix up conditional_replacement [PR99305]
2021-03-09 15:40 Jakub Jelinek
2021-03-09 17:26 ` Richard Biener
@ 2021-03-09 17:46 ` Jeff Law
1 sibling, 0 replies; 4+ messages in thread
From: Jeff Law @ 2021-03-09 17:46 UTC (permalink / raw)
To: Jakub Jelinek, Richard Biener; +Cc: gcc-patches
On 3/9/21 8:40 AM, Jakub Jelinek via Gcc-patches wrote:
> Hi!
>
> Before my PR97690 changes, conditional_replacement would not set neg
> when the nonzero arg was boolean true.
> I've simplified the testing, so that it first finds the zero argument
> and then checks the other argument for all the handled cases
> (1, -1 and 1 << X, where the last case is what the patch added support for).
> But, unfortunately I've placed the integer_all_onesp test first.
> For unsigned precision 1 types such as bool integer_all_onesp, integer_onep
> and integer_pow2p can all be true and the code set neg to true in that case,
> which is undesirable.
>
> The following patch tests integer_pow2p first (which is trivially true
> for integer_onep too and tree_log2 in that case gives shift == 0)
> and only if that isn't the case, integer_all_onesp.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2021-03-09 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/99305
> * tree-ssa-phiopt.c (conditional_replacement): Test integer_pow2p
> before integer_all_onesp instead of vice versa.
>
> * g++.dg/opt/pr99305.C: New test.
OK
jeff
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] phiopt: Fix up conditional_replacement [PR99305]
2021-03-09 15:40 Jakub Jelinek
@ 2021-03-09 17:26 ` Richard Biener
2021-03-09 17:46 ` Jeff Law
1 sibling, 0 replies; 4+ messages in thread
From: Richard Biener @ 2021-03-09 17:26 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On March 9, 2021 4:40:22 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>Before my PR97690 changes, conditional_replacement would not set neg
>when the nonzero arg was boolean true.
>I've simplified the testing, so that it first finds the zero argument
>and then checks the other argument for all the handled cases
>(1, -1 and 1 << X, where the last case is what the patch added support
>for).
>But, unfortunately I've placed the integer_all_onesp test first.
>For unsigned precision 1 types such as bool integer_all_onesp,
>integer_onep
>and integer_pow2p can all be true and the code set neg to true in that
>case,
>which is undesirable.
>
>The following patch tests integer_pow2p first (which is trivially true
>for integer_onep too and tree_log2 in that case gives shift == 0)
>and only if that isn't the case, integer_all_onesp.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok.
Richard.
>2021-03-09 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/99305
> * tree-ssa-phiopt.c (conditional_replacement): Test integer_pow2p
> before integer_all_onesp instead of vice versa.
>
> * g++.dg/opt/pr99305.C: New test.
>
>--- gcc/tree-ssa-phiopt.c.jj 2021-01-22 11:41:38.078708425 +0100
>+++ gcc/tree-ssa-phiopt.c 2021-03-09 13:15:02.649094949 +0100
>@@ -808,14 +808,14 @@ conditional_replacement (basic_block con
> nonzero_arg = arg0;
> else
> return false;
>- if (integer_all_onesp (nonzero_arg))
>- neg = true;
>- else if (integer_pow2p (nonzero_arg))
>+ if (integer_pow2p (nonzero_arg))
> {
> shift = tree_log2 (nonzero_arg);
> if (shift && POINTER_TYPE_P (TREE_TYPE (nonzero_arg)))
> return false;
> }
>+ else if (integer_all_onesp (nonzero_arg))
>+ neg = true;
> else
> return false;
>
>--- gcc/testsuite/g++.dg/opt/pr99305.C.jj 2021-03-09 13:39:11.040957563
>+0100
>+++ gcc/testsuite/g++.dg/opt/pr99305.C 2021-03-09 13:39:44.063589691
>+0100
>@@ -0,0 +1,26 @@
>+// PR tree-optimization/99305
>+// { dg-do compile }
>+// { dg-options "-O3 -fno-ipa-icf -fdump-tree-optimized" }
>+// { dg-final { scan-tree-dump-times " = \\\(unsigned char\\\)
>c_\[0-9]*\\\(D\\\);" 3 "optimized" } }
>+// { dg-final { scan-tree-dump-times " = \[^\n\r]* \\+ \[0-9]*;" 3
>"optimized" } }
>+// { dg-final { scan-tree-dump-times " = \[^\n\r]* <= 9;" 3
>"optimized" } }
>+// { dg-final { scan-tree-dump-not "if \\\(c_\[0-9]*\\\(D\\\) \[!=]=
>0\\\)" "optimized" } }
>+// { dg-final { scan-tree-dump-not " = PHI <" "optimized" } }
>+
>+bool
>+foo (char c)
>+{
>+ return c >= 48 && c <= 57;
>+}
>+
>+bool
>+bar (char c)
>+{
>+ return c != 0 && foo (c);
>+}
>+
>+bool
>+baz (char c)
>+{
>+ return c != 0 && c >= 48 && c <= 57;
>+}
>
> Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] phiopt: Fix up conditional_replacement [PR99305]
@ 2021-03-09 15:40 Jakub Jelinek
2021-03-09 17:26 ` Richard Biener
2021-03-09 17:46 ` Jeff Law
0 siblings, 2 replies; 4+ messages in thread
From: Jakub Jelinek @ 2021-03-09 15:40 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
Hi!
Before my PR97690 changes, conditional_replacement would not set neg
when the nonzero arg was boolean true.
I've simplified the testing, so that it first finds the zero argument
and then checks the other argument for all the handled cases
(1, -1 and 1 << X, where the last case is what the patch added support for).
But, unfortunately I've placed the integer_all_onesp test first.
For unsigned precision 1 types such as bool integer_all_onesp, integer_onep
and integer_pow2p can all be true and the code set neg to true in that case,
which is undesirable.
The following patch tests integer_pow2p first (which is trivially true
for integer_onep too and tree_log2 in that case gives shift == 0)
and only if that isn't the case, integer_all_onesp.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2021-03-09 Jakub Jelinek <jakub@redhat.com>
PR tree-optimization/99305
* tree-ssa-phiopt.c (conditional_replacement): Test integer_pow2p
before integer_all_onesp instead of vice versa.
* g++.dg/opt/pr99305.C: New test.
--- gcc/tree-ssa-phiopt.c.jj 2021-01-22 11:41:38.078708425 +0100
+++ gcc/tree-ssa-phiopt.c 2021-03-09 13:15:02.649094949 +0100
@@ -808,14 +808,14 @@ conditional_replacement (basic_block con
nonzero_arg = arg0;
else
return false;
- if (integer_all_onesp (nonzero_arg))
- neg = true;
- else if (integer_pow2p (nonzero_arg))
+ if (integer_pow2p (nonzero_arg))
{
shift = tree_log2 (nonzero_arg);
if (shift && POINTER_TYPE_P (TREE_TYPE (nonzero_arg)))
return false;
}
+ else if (integer_all_onesp (nonzero_arg))
+ neg = true;
else
return false;
--- gcc/testsuite/g++.dg/opt/pr99305.C.jj 2021-03-09 13:39:11.040957563 +0100
+++ gcc/testsuite/g++.dg/opt/pr99305.C 2021-03-09 13:39:44.063589691 +0100
@@ -0,0 +1,26 @@
+// PR tree-optimization/99305
+// { dg-do compile }
+// { dg-options "-O3 -fno-ipa-icf -fdump-tree-optimized" }
+// { dg-final { scan-tree-dump-times " = \\\(unsigned char\\\) c_\[0-9]*\\\(D\\\);" 3 "optimized" } }
+// { dg-final { scan-tree-dump-times " = \[^\n\r]* \\+ \[0-9]*;" 3 "optimized" } }
+// { dg-final { scan-tree-dump-times " = \[^\n\r]* <= 9;" 3 "optimized" } }
+// { dg-final { scan-tree-dump-not "if \\\(c_\[0-9]*\\\(D\\\) \[!=]= 0\\\)" "optimized" } }
+// { dg-final { scan-tree-dump-not " = PHI <" "optimized" } }
+
+bool
+foo (char c)
+{
+ return c >= 48 && c <= 57;
+}
+
+bool
+bar (char c)
+{
+ return c != 0 && foo (c);
+}
+
+bool
+baz (char c)
+{
+ return c != 0 && c >= 48 && c <= 57;
+}
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-10 16:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-10 16:25 [PATCH] phiopt: Fix up conditional_replacement [PR99305] David Edelsohn
-- strict thread matches above, loose matches on Subject: below --
2021-03-09 15:40 Jakub Jelinek
2021-03-09 17:26 ` Richard Biener
2021-03-09 17:46 ` Jeff Law
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).