Re: [RFC] optimize x - y cmp 0 with undefined overflow

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, May 27, 2014 at 11:25 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> +      tree new_const
>> +       = fold_build2_loc (loc, reverse_op, TREE_TYPE (arg1), const2,
>> const1);
>>
>>        /* If the constant operation overflowed this can be
>>          simplified as a comparison against INT_MAX/INT_MIN.  */
>> -      if (TREE_CODE (lhs) == INTEGER_CST
>> -         && TREE_OVERFLOW (lhs))
>> +      if (TREE_OVERFLOW (new_const))
>>
>> well, either use int_const_binop above or retain the check (or use
>> TREE_OVERFLOW_P).  Bonus points for using wide-ints here
>> and not relying on TREE_OVERFLOW.
>
> The check is useless (you get either NULL_TREE or INTEGER_CST here) but I'll
> use int_const_binop.
>
>> +  /* Transform comparisons of the form X - Y CMP 0 to X CMP Y.  */
>> +  if (TREE_CODE (arg0) == MINUS_EXPR
>> +      && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (arg1))
>>
>> any good reason for using TYPE_OVERFLOW_UNDEFINED on the
>> type of arg1 instead on the type of the MINUS (yes, they should
>> match, but it really looks odd ... the overflow of the minus has to be
>> undefined).
>
> For the sake of consistency with the X +- C1 CMP C2 case just above, but I can
> change both.
>
>> Also for EQ_EXPR and NE_EXPR the transform is
>> fine even when !TYPE_OVERFLOW_UNDEFINED (and we seem
>> to perform it already somewhere).  Please look where and try to
>> add the undefined overflow case to it.
>
> Yes, but it's the same for the X +- C1 CMP C2 case, i.e. there are specific
> cases for X +- C1 EQ/NE C2 and X - Y EQ/NE 0 in fold_binary, so I'm not sure
> what you're asking.

I'm asking to merge them (move them to fold_comparison).

>> As for the VRP pieces I don't understand what is missing here
>> (well, compare_range_with_value and/or compare_values might
>> not handle this case?  then better fix that to improve symbolic
>> range handling in general?).  Also I have a longstanding patch
>> in my tree that does
>
> Yes, there is an explicit non-handling of symbolic ranges for PLUS_EXPR and
> MINUS_EXPR in VRP (extract_range_from_binary_expr_1) and the patch works
> around it by propagating the code instead of the ranges, which is far easier
> and sufficient here.  If you think that the way to go is to handle symbolic
> ranges for PLUS_EXPR and MINUS_EXPR instead, fine with me, I can try.

Yeah, it would be nice to see some support.  The most interesting cases
will be symbolic-singleton +- CST where the offset shrinks a constant offset
in a symbolic A +- CST (thus we don't get into any overflow issues).  Thus
handling

 [a + 1, a + 1] - [1, 1] -> [a, a]

for example.  We get the offsetted singleton symbolic ranges from
conditional asserts a lot.

Thanks,
Richard.

> --
> Eric Botcazou