Re: [PATCH] Tree-level fix for PR 69526

["Bin.Cheng" <amker dot cheng at gmail dot com>]

On Tue, Jan 17, 2017 at 9:48 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Tue, Jan 10, 2017 at 2:32 PM, Robin Dapp <rdapp@linux.vnet.ibm.com> wrote:
>> Perhaps I'm still missing how some cases are handled or not handled,
>> sorry for the noise.
>>
>>> I'm not sure there is anything to "interpret" -- the operation is unsigned
>>> and overflow is when the operation may wrap around zero.  There might
>>> be clever ways of re-writing the expression to
>>> (uint64_t)((uint32_t)((int32_t)uint32 + -1) + 1)
>>> avoiding the overflow and thus allowing the transform but I'm not sure that's
>>> good.
>>
>> The extra work I introduced was to discern between
>>
>>  (uint64_t)(a + UINT_MAX) + 1  -> (uint64_t)(a),
>>  (uint64_t)(a + UINT_MAX) + 1  -> (uint64_t)(a) + (uint64_t)(UINT_MAX + 1),
>>
>> For a's range of [1,1] there is an overflow in both cases.
>> We still want to simplify the first case by combining UINT_MAX + 1 -> 0.
>
> So there's the case where a == 0 where (uint64_t)(0 + UINT_MAX) + 1 is not zero.
>
> I think we're still searching for the proper condition on when it is allowed to
> re-write
>
>  (uint64_t)(uint32_t + uint32_t-CST) + uint64_t-CST
>
> to
>
>  (uint64_t)(uint32_t) + (uint64_t)uint32_t-CST + uint64_t-CST
Hmm, won't (uint32_t + uint32_t-CST) doesn't overflow be sufficient
condition for such transformation?
>
> or to
>
>  (uint64_t)(uint32_t) + (uint64_t)(uint32_t-CST + (uint32_t)uint64_t-CST)
We don't actually need to prove this?  What complicates this is when
it's safe to transform:
 (uint64_t)(uint32_t + uint32_t-CST) + uint64_t-CST
into
 (uint64_t)(uint32_t + uint32_t-CSTx)
where
 uint32_t-CSTx = uint32_t-CST + (uint32_t)uint64_t-CST

Am I misunderstanding the whole thing?

Thanks,
bin
>
>> If "interpreting" UINT_MAX as -1 is not the correct thing to do, perhaps
>> (uint64_t)((uint32_t)(UINT_MAX + 1)) is? This fails, however, if the
>> outer constant is larger than UINT_MAX. What else can we do here?
>> Do we see cases like the second one at all? If it's not needed, the
>> extra work is likely not needed.
>
> We do have the need to associate and simplfy across conversions but of
> course we have to do it conservatively correct.
>
>>> A related thing would be canonicalizing unsigned X plus CST to
>>> unsigned X minus CST'
>>> if CST' has a smaller absolute value than CST.  I think currently we
>>> simply canonicalize
>>> to 'plus CST' but also only in fold-const.c, not in match.pd (ah we
>>> do, but only in a simplified manner).
>>
>> I can imagine this could simplify the treatment of some cases, yet I'm
>> already a bit lost with the current cases :)
>
> Yes, but I am lost a bit as well.  I don't know the correct conditions to test
> off-head -- I know we may not introduce new undefined overflow ops and
> of course we need to not compute wrong numbers either.
>
>>> That said, can we leave that "trick" out of the patch?  I think your
>>> more complicated
>>> "overflows" result from extract_range_from_binary_expr_1 doesn't apply to all
>>> ops (like MULT_EXPR where more complicated cases can arise).
>>
>> There is certainly additional work to be done for MULT_EXPR, I
>> disregarded it so far. For this patch, I'd rather conservatively assume
>> overflow.
>
> Ok...
>
> Richard.
>
>> Regards
>>  Robin
>>