On 23/05/16 15:06, Richard Biener wrote:
On Mon, May 23, 2016 at 3:19 PM, Kyrill Tkachov
<kyrylo.tkac...@foss.arm.com> wrote:
On 23/05/16 13:46, Richard Biener wrote:
n Mon, May 23, 2016 at 2:28 PM, Kyrill Tkachov
<kyrylo.tkac...@foss.arm.com> wrote:
On 23/05/16 12:27, Richard Biener wrote:
On Mon, May 23, 2016 at 1:17 PM, Kyrill Tkachov
<kyrylo.tkac...@foss.arm.com> wrote:
Hi all,
In this PR we end up hitting a signed overflow in
noce_get_alt_condition
when we try to
increment or decrement a HOST_WIDE_INT that might be HOST_WIDE_INT_MAX
or
HOST_WIDE_INT_MIN.
I've confirmed the overflow by adding an assert before the operation:
gcc_assert (desired_val != HOST_WIDE_INT_MAX);
This patch fixes those cases by catching the cases when desired_val has
the
extreme
value and avoids the transformation that function is trying to make.
Bootstrapped and tested on arm, aarch64, x86_64.
I've added the testcase that I used to trigger the assert mentioned
above
as
a compile test,
though I'm not sure how much value it has...
Ok for trunk?
If this isn't also a wrong-code issue (runtime testcase?) then why not
perform
the operation in unsigned HOST_WIDE_INT instead?
This part of the code transforms a comparison "x < CST" to "x <= CST - 1"
and similar transformations. Fro what I understand the LT,LE,GT,GE RTL
comparison
operators operate on signed integers, so I'm not sure how valid it would
be
to do all this on unsigned HOST_WIDE_INT.
But then this is a wrong-code issue and you should see miscompiles
and thus can add a dg-do run testcase instead?
I couldn't get it to miscompile anything, because the check:
"actual_val == desired_val + 1" where desired_val + 1 has signed
overflow doesn't return true, so the transformation doesn't happen anyway.
I think whether a miscompilation can occur depends on whether the compiler
used
to compile GCC itself does anything funky with the undefined behaviour
that's
occurring, which is why we should fix it. I suppose the testcase in this
patch only
goes so far to show that GCC doesn't crash, but not much else. I can make it
an execute
testcase if you'd like, but I can't get it to fail on my setup (the
generated assembly
looks correct on inspection).
Please make it a execute testcase anyway.
Ok with that change.
Thanks Richard, here it is with the testcase made executable.
Committing to trunk.
Kyrill
2016-05-25 Kyrylo Tkachov <kyrylo.tkac...@arm.com>
PR rtl-optimization/66940
* ifcvt.c (noce_get_alt_condition): Check that incrementing or
decrementing desired_val will not overflow before performing these
operations.
2016-05-25 Kyrylo Tkachov <kyrylo.tkac...@arm.com>
PR rtl-optimization/66940
* gcc.c-torture/execute/pr66940.c: New test.
Thanks,
Richard.
Kyrill
Kyrill
Richard.
Thanks,
Kyrill
Richard.
Thanks,
Kyrill
2016-05-23 Kyrylo Tkachov <kyrylo.tkac...@arm.com>
PR rtl-optimization/66940
* ifcvt.c (noce_get_alt_condition): Check that incrementing or
decrementing desired_val will not overflow before performing
these
operations.
2016-05-23 Kyrylo Tkachov <kyrylo.tkac...@arm.com>
PR rtl-optimization/66940
* gcc.c-torture/compile/pr66940.c: New test.
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 6ce51ba70464ce8ec79e217d290c2ab14f317651..cab054889aea7fafab7b5310da45ef7b3fc7e50b 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2416,28 +2416,32 @@ noce_get_alt_condition (struct noce_if_info *if_info, rtx target,
switch (code)
{
case LT:
- if (actual_val == desired_val + 1)
+ if (desired_val != HOST_WIDE_INT_MAX
+ && actual_val == desired_val + 1)
{
code = LE;
op_b = GEN_INT (desired_val);
}
break;
case LE:
- if (actual_val == desired_val - 1)
+ if (desired_val != HOST_WIDE_INT_MIN
+ && actual_val == desired_val - 1)
{
code = LT;
op_b = GEN_INT (desired_val);
}
break;
case GT:
- if (actual_val == desired_val - 1)
+ if (desired_val != HOST_WIDE_INT_MIN
+ && actual_val == desired_val - 1)
{
code = GE;
op_b = GEN_INT (desired_val);
}
break;
case GE:
- if (actual_val == desired_val + 1)
+ if (desired_val != HOST_WIDE_INT_MAX
+ && actual_val == desired_val + 1)
{
code = GT;
op_b = GEN_INT (desired_val);
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr66940.c b/gcc/testsuite/gcc.c-torture/execute/pr66940.c
new file mode 100644
index 0000000000000000000000000000000000000000..fbd109dccc72c290d935d81a3f3dd219f11f908d
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr66940.c
@@ -0,0 +1,20 @@
+long long __attribute__ ((noinline, noclone))
+foo (long long ival)
+{
+ if (ival <= 0)
+ return -0x7fffffffffffffffL - 1;
+
+ return 0x7fffffffffffffffL;
+}
+
+int
+main (void)
+{
+ if (foo (-1) != (-0x7fffffffffffffffL - 1))
+ __builtin_abort ();
+
+ if (foo (1) != 0x7fffffffffffffffL)
+ __builtin_abort ();
+
+ return 0;
+}
