| Message ID | 1d5dd212-cd02-6b6d-0e89-20c7e96f3a02@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Combine logical OR ranges properly. pr97567 | expand |
Hi, On Mon, 26 Oct 2020 at 22:51, Andrew MacLeod via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > In the core of gori_compute::logical_combine we are suppose to combine > the calculated true and false ranges on each side of the operation. > > when encountering > > [0,0] = c_3 | c_4 > > we know we only need to consider the FALSE values of the range carried > by c_3 and c_4, but it can be EITHER of those ranges, so we need to > union them together to get the correct result. > > The code was performing an intersection instead, and in this particualr > case, we knew the range carried thru c_3 was alwasy [0,0] and it was > always varying through c_4.... instead of returning varying, we were > returning [0,0] which then caused some folding which was incorrect. > > Fixed by correctly calling union... > > Bootstrapped on x86_64-pc-linux-gnu, no regressions, and pushed. > I think you need to update the testcase and declare long long g = 4073709551615 instead of just long, as it causes a warning on 32-bit targets: /gcc/testsuite/gcc.dg/pr97567.c:7:12: warning: overflow in conversion from 'long long int' to 'long int' changes value from '4073709551615' to '2080555007' [-Woverflow] Christophe > Andrew >
On Tue, Oct 27, 2020 at 4:24 AM Christophe Lyon via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Hi, > > On Mon, 26 Oct 2020 at 22:51, Andrew MacLeod via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > In the core of gori_compute::logical_combine we are suppose to combine > > the calculated true and false ranges on each side of the operation. > > > > when encountering > > > > [0,0] = c_3 | c_4 > > > > we know we only need to consider the FALSE values of the range carried > > by c_3 and c_4, but it can be EITHER of those ranges, so we need to > > union them together to get the correct result. > > > > The code was performing an intersection instead, and in this particualr > > case, we knew the range carried thru c_3 was alwasy [0,0] and it was > > always varying through c_4.... instead of returning varying, we were > > returning [0,0] which then caused some folding which was incorrect. > > > > Fixed by correctly calling union... > > > > Bootstrapped on x86_64-pc-linux-gnu, no regressions, and pushed. > > > > I think you need to update the testcase and declare > long long g = 4073709551615 > instead of just long, as it causes a warning on 32-bit targets: > /gcc/testsuite/gcc.dg/pr97567.c:7:12: warning: overflow in conversion > from 'long long int' to 'long int' changes value from '4073709551615' > to '2080555007' [-Woverflow] > The testcase is an infinite loop on ILP32 targets.
On 10/27/20 7:23 AM, Christophe Lyon wrote: > Hi, > > On Mon, 26 Oct 2020 at 22:51, Andrew MacLeod via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> In the core of gori_compute::logical_combine we are suppose to combine >> the calculated true and false ranges on each side of the operation. >> >> when encountering >> >> [0,0] = c_3 | c_4 >> >> we know we only need to consider the FALSE values of the range carried >> by c_3 and c_4, but it can be EITHER of those ranges, so we need to >> union them together to get the correct result. >> >> The code was performing an intersection instead, and in this particualr >> case, we knew the range carried thru c_3 was alwasy [0,0] and it was >> always varying through c_4.... instead of returning varying, we were >> returning [0,0] which then caused some folding which was incorrect. >> >> Fixed by correctly calling union... >> >> Bootstrapped on x86_64-pc-linux-gnu, no regressions, and pushed. >> > I think you need to update the testcase and declare > long long g = 4073709551615 > instead of just long, as it causes a warning on 32-bit targets: > /gcc/testsuite/gcc.dg/pr97567.c:7:12: warning: overflow in conversion > from 'long long int' to 'long int' changes value from '4073709551615' > to '2080555007' [-Woverflow] > > Christophe > >> Andrew >> ah, didn't realize the testcase didnt work properly on non 64 bit targets... I'll switch it to long long, that seems to make it work. thanks Andrew commit 3af44504d40d688cafc43d1b850a55ef794b443a Author: Andrew MacLeod <amacleod@redhat.com> Date: Tue Oct 27 10:13:18 2020 -0400 Combine logical OR ranges properly. pr97567 update testcase to work on 32 bit targets gcc/testsuite * gcc.dg/pr97567.c: Update to work with 32 bit targets. diff --git a/gcc/testsuite/gcc.dg/pr97567.c b/gcc/testsuite/gcc.dg/pr97567.c index b2b72a4d2a7..8922f277214 100644 --- a/gcc/testsuite/gcc.dg/pr97567.c +++ b/gcc/testsuite/gcc.dg/pr97567.c @@ -4,7 +4,7 @@ int a, b, c, d; void k() { unsigned f = 1; - long g = 4073709551615; + long long g = 4073709551615; for (; a; a++) for (;;) { d = 0; @@ -16,7 +16,7 @@ void k() { ; g || f; int i = 0 - f || g; - long j = g - f; + long long j = g - f; if (j || f) { if (g < 4073709551615) for (;;)
On Tue, Oct 27, 2020 at 7:18 AM Andrew MacLeod via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On 10/27/20 7:23 AM, Christophe Lyon wrote: > > Hi, > > > > On Mon, 26 Oct 2020 at 22:51, Andrew MacLeod via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > >> In the core of gori_compute::logical_combine we are suppose to combine > >> the calculated true and false ranges on each side of the operation. > >> > >> when encountering > >> > >> [0,0] = c_3 | c_4 > >> > >> we know we only need to consider the FALSE values of the range carried > >> by c_3 and c_4, but it can be EITHER of those ranges, so we need to > >> union them together to get the correct result. > >> > >> The code was performing an intersection instead, and in this particualr > >> case, we knew the range carried thru c_3 was alwasy [0,0] and it was > >> always varying through c_4.... instead of returning varying, we were > >> returning [0,0] which then caused some folding which was incorrect. > >> > >> Fixed by correctly calling union... > >> > >> Bootstrapped on x86_64-pc-linux-gnu, no regressions, and pushed. > >> > > I think you need to update the testcase and declare > > long long g = 4073709551615 > > instead of just long, as it causes a warning on 32-bit targets: > > /gcc/testsuite/gcc.dg/pr97567.c:7:12: warning: overflow in conversion > > from 'long long int' to 'long int' changes value from '4073709551615' > > to '2080555007' [-Woverflow] > > > > Christophe > > > >> Andrew > >> > ah, didn't realize the testcase didnt work properly on non 64 bit > targets... I'll switch it to long long, that seems to make it work. > It works for me. Can you check it in to unblock my testers? Thanks.
On 10/27/20 10:44 AM, H.J. Lu wrote: > On Tue, Oct 27, 2020 at 7:18 AM Andrew MacLeod via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> On 10/27/20 7:23 AM, Christophe Lyon wrote: >>> Hi, >>> >>> On Mon, 26 Oct 2020 at 22:51, Andrew MacLeod via Gcc-patches >>> <gcc-patches@gcc.gnu.org> wrote: >>>> In the core of gori_compute::logical_combine we are suppose to combine >>>> the calculated true and false ranges on each side of the operation. >>>> >>>> when encountering >>>> >>>> [0,0] = c_3 | c_4 >>>> >>>> we know we only need to consider the FALSE values of the range carried >>>> by c_3 and c_4, but it can be EITHER of those ranges, so we need to >>>> union them together to get the correct result. >>>> >>>> The code was performing an intersection instead, and in this particualr >>>> case, we knew the range carried thru c_3 was alwasy [0,0] and it was >>>> always varying through c_4.... instead of returning varying, we were >>>> returning [0,0] which then caused some folding which was incorrect. >>>> >>>> Fixed by correctly calling union... >>>> >>>> Bootstrapped on x86_64-pc-linux-gnu, no regressions, and pushed. >>>> >>> I think you need to update the testcase and declare >>> long long g = 4073709551615 >>> instead of just long, as it causes a warning on 32-bit targets: >>> /gcc/testsuite/gcc.dg/pr97567.c:7:12: warning: overflow in conversion >>> from 'long long int' to 'long int' changes value from '4073709551615' >>> to '2080555007' [-Woverflow] >>> >>> Christophe >>> >>>> Andrew >>>> >> ah, didn't realize the testcase didnt work properly on non 64 bit >> targets... I'll switch it to long long, that seems to make it work. >> > It works for me. Can you check it in to unblock my testers? > > Thanks. > should be all checked in already. sorry I wasnt clear
commit 48722d158cbf692c24025e345ecbbbb570f66aa5
Author: Andrew MacLeod <amacleod@redhat.com>
Date: Mon Oct 26 14:55:00 2020 -0400
Combine logical OR ranges properly.
When combining logical OR operands with a FALSE result, union the false
ranges for operand1 and operand2... not intersection.
gcc/
PR tree-optimization/97567
* gimple-range-gori.cc (gori_compute::logical_combine): Union the
ranges of operand1 and operand2, not intersect.
gcc/testsuite/
* gcc.dg/pr97567.c: New.
diff --git a/gcc/gimple-range-gori.cc b/gcc/gimple-range-gori.cc
index 5d50b111d2a..de0f653860d 100644
--- a/gcc/gimple-range-gori.cc
+++ b/gcc/gimple-range-gori.cc
@@ -730,10 +730,10 @@ gori_compute::logical_combine (irange &r, enum tree_code code,
if (lhs.zero_p ())
{
// An OR operation will only take the FALSE path if both
- // operands are false, so [20, 255] intersect [0, 5] is the
+ // operands are false, so either [20, 255] or [0, 5] is the
// union: [0,5][20,255].
r = op1.false_range;
- r.intersect (op2.false_range);
+ r.union_ (op2.false_range);
}
else
{
diff --git a/gcc/testsuite/gcc.dg/pr97567.c b/gcc/testsuite/gcc.dg/pr97567.c
new file mode 100644
index 00000000000..b2b72a4d2a7
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr97567.c
@@ -0,0 +1,34 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+int a, b, c, d;
+void k() {
+ unsigned f = 1;
+ long g = 4073709551615;
+ for (; a; a++)
+ for (;;) {
+ d = 0;
+ L1:
+ break;
+ }
+ if (f)
+ for (; a; a++)
+ ;
+ g || f;
+ int i = 0 - f || g;
+ long j = g - f;
+ if (j || f) {
+ if (g < 4073709551615)
+ for (;;)
+ ;
+ int e = ~f, h = b / ~e;
+ if (c)
+ goto L2;
+ g = f = h;
+ }
+ g || d;
+L2:
+ if (c)
+ goto L1;
+}
+int main() { k(); return 0; }