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. > 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. >>> >>> >
