On 09/04/2018 07:58 AM, Richard Biener wrote:
On Mon, Sep 3, 2018 at 1:32 PM Aldy Hernandez <al...@redhat.com> wrote:
On 08/28/2018 05:27 AM, Richard Biener wrote:
On Mon, Aug 27, 2018 at 2:24 PM Aldy Hernandez <al...@redhat.com> wrote:
Howdy!
Phew, I think this is the last abstraction. This handles the unary
CONVERT_EXPR_P code.
It's the usual story-- normalize the symbolics to [-MIN,+MAX] and handle
everything generically.
Normalizing the symbolics brought about some nice surprises. We now
handle a few things we were punting on before, which I've documented in
the patch, but can remove if so desired. I wrote them mainly for myself:
/* NOTES: Previously we were returning VARYING for all symbolics, but
we can do better by treating them as [-MIN, +MAX]. For
example, converting [SYM, SYM] from INT to LONG UNSIGNED,
we can return: ~[0x8000000, 0xffffffff7fffffff].
We were also failing to convert ~[0,0] from char* to unsigned,
instead choosing to return VR_VARYING. Now we return ~[0,0]. */
Tested on x86-64 by the usual bootstrap and regtest gymnastics,
including --enable-languages=all, because my past sins are still
haunting me.
OK?
The new wide_int_range_convert_tree looks odd given it returns
tree's. I'd have expected an API that does the conversion resulting
in a wide_int range and the VRP code adapting to that by converting
the result to trees.
Rewritten as suggested.
A few notes.
First. I am not using widest_int as was done previously. We were
passing 0/false to the overflow args in force_fit_type so the call was
just degrading into wide_int_to_tree() anyhow. Am I missing some
subtlety here, and must we use widest_int and then force_fit_type on the
caller?
The issue is that the wide-ints get "converted", so it's indeed subtle.
This seems like it should be documented-- at the very least in
wide_int_range_convert.
What do you mean "converted"? I'd like to understand this better before
I write out the comment :). Do we lose precision by keeping it in a
wide_int as opposed to a widest_int?
Also, can we have the caller to wide_int_range_convert() use
wide_int_to_tree directly instead of passing through force_fit_type? It
seems force_fit_type with overflowable=0 and overflowed=false is just a
call to wide_int_to_tree anyhow.
+ || wi::rshift (wi::sub (vr0_max, vr0_min),
+ wi::uhwi (outer_prec, inner_prec),
+ inner_sign) == 0)
this was previously allowed only for VR_RANGEs - now it's also for
VR_ANTI_RANGE.
That looks incorrect. Testcase welcome ;)
See change to extract_range_into_wide_ints. VR_ANTI_RANGEs of constants
will remain as is, whereas VR_ANTI_RANGEs of symbols will degrade into
[-MIN,+MAX] and be handled generically.
Also, see comment:
+ /* We normalize everything to a VR_RANGE, but for constant
+ anti-ranges we must handle them by leaving the final result
+ as an anti range. This allows us to convert things like
+ ~[0,5] seamlessly. */
+ return (!wi::eq_p (min, wi::min_value (outer_prec, outer_sign))
+ || !wi::eq_p (max, wi::max_value (outer_prec, outer_sign)));
so you return false for VARYING? Why? I'd simply return true and
return VARYING in the new type in the path you return false.
Since symbols and VR_VARYING and other things get turned into a
[-MIN,+MAX] by extract_range_into_wide_ints, it's a lot easier to handle
things generically and return varying when the range spans the entire
domain. It also keeps with the rest of the wide_int_range_* functions
that return false when we know it's going to be VR_VARYING.
Aldy
Richard.
Second. I need extract_range_into_wide_ints to work with anti ranges of
constants. It seems like only the unary handling of CONVERT_EXPR here
uses it that way, so I believe it should go into one patch. If you
believe strongly otherwise, I could split out the 4 lines into a
separate patch, but I'd prefer not to.
Finally, I could use vr0_min.get_precision() instead of inner_precision,
but I think it's more regular and readable the way I have it.
Tested on all languages on x86-64 Linux.
OK for trunk?
