| Message ID | b9f2d574-4dca-360e-a85c-c58b446dfd19@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Implement {get,set}_range_info() variants that work with value_range's | expand |
On Thu, Nov 8, 2018 at 12:52 PM Aldy Hernandez <aldyh@redhat.com> wrote: > > get/set_range_info() currently returns the extremes of a range. I have > implemented overloaded variants that return a proper range. In the > future we should use actual ranges throughout, and not depend on range > extremes, as depending on this behavior could causes us to lose precision. > > I am also including changes to size_must_be_zero_p() to show how we > should be using the range API, as opposed to performing error prone > ad-hoc calculations on ranges and anti-ranges. Yeah, I've been talking this all along but not being heard... > Martin, I'm not saying your code is wrong. There are numerous other > places in the compiler where we manipulate ranges/anti-ranges directly, > all of which should be adapted in the future. Everywhere there is a > mention of VR_RANGE/VR_ANTI_RANGE in the compiler is suspect. We should > ideally be using intersect/union/may_contain_p/null_p, etc. null_p is a bad name btw, I just confused it with empty_p ... (which we have as undefined_p). contains_only_zero_p would be less confusing. > OK pending another round of tests? (As I had tested this patch along > with a whole slew of other upcoming changes ;-)). OK. Richard. > > Aldy
On 11/8/18 8:59 AM, Richard Biener wrote: > On Thu, Nov 8, 2018 at 12:52 PM Aldy Hernandez <aldyh@redhat.com> wrote: >> >> get/set_range_info() currently returns the extremes of a range. I have >> implemented overloaded variants that return a proper range. In the >> future we should use actual ranges throughout, and not depend on range >> extremes, as depending on this behavior could causes us to lose precision. >> >> I am also including changes to size_must_be_zero_p() to show how we >> should be using the range API, as opposed to performing error prone >> ad-hoc calculations on ranges and anti-ranges. > > Yeah, I've been talking this all along but not being heard... My girlfriend says I don't listen. It could be related. >> Martin, I'm not saying your code is wrong. There are numerous other >> places in the compiler where we manipulate ranges/anti-ranges directly, >> all of which should be adapted in the future. Everywhere there is a >> mention of VR_RANGE/VR_ANTI_RANGE in the compiler is suspect. We should >> ideally be using intersect/union/may_contain_p/null_p, etc. > > null_p is a bad name btw, I just confused it with empty_p ... (which we have > as undefined_p). contains_only_zero_p would be less confusing. Yes, a horrible name. I noticed so as I debugged precisely this bit. How about zero_p? Aldy
On Thu, Nov 8, 2018 at 3:05 PM Aldy Hernandez <aldyh@redhat.com> wrote: > > > > On 11/8/18 8:59 AM, Richard Biener wrote: > > On Thu, Nov 8, 2018 at 12:52 PM Aldy Hernandez <aldyh@redhat.com> wrote: > >> > >> get/set_range_info() currently returns the extremes of a range. I have > >> implemented overloaded variants that return a proper range. In the > >> future we should use actual ranges throughout, and not depend on range > >> extremes, as depending on this behavior could causes us to lose precision. > >> > >> I am also including changes to size_must_be_zero_p() to show how we > >> should be using the range API, as opposed to performing error prone > >> ad-hoc calculations on ranges and anti-ranges. > > > > Yeah, I've been talking this all along but not being heard... > > My girlfriend says I don't listen. It could be related. > > >> Martin, I'm not saying your code is wrong. There are numerous other > >> places in the compiler where we manipulate ranges/anti-ranges directly, > >> all of which should be adapted in the future. Everywhere there is a > >> mention of VR_RANGE/VR_ANTI_RANGE in the compiler is suspect. We should > >> ideally be using intersect/union/may_contain_p/null_p, etc. > > > > null_p is a bad name btw, I just confused it with empty_p ... (which we have > > as undefined_p). contains_only_zero_p would be less confusing. > > Yes, a horrible name. I noticed so as I debugged precisely this bit. > How about zero_p? Probably the same ambiguous connotation? But yes, way better than null_p. Richard. > Aldy
On 11/8/18 9:41 AM, Richard Biener wrote: > On Thu, Nov 8, 2018 at 3:05 PM Aldy Hernandez <aldyh@redhat.com> wrote: >> >> >> >> On 11/8/18 8:59 AM, Richard Biener wrote: >>> On Thu, Nov 8, 2018 at 12:52 PM Aldy Hernandez <aldyh@redhat.com> wrote: >>>> >>>> get/set_range_info() currently returns the extremes of a range. I have >>>> implemented overloaded variants that return a proper range. In the >>>> future we should use actual ranges throughout, and not depend on range >>>> extremes, as depending on this behavior could causes us to lose precision. >>>> >>>> I am also including changes to size_must_be_zero_p() to show how we >>>> should be using the range API, as opposed to performing error prone >>>> ad-hoc calculations on ranges and anti-ranges. >>> >>> Yeah, I've been talking this all along but not being heard... >> >> My girlfriend says I don't listen. It could be related. >> >>>> Martin, I'm not saying your code is wrong. There are numerous other >>>> places in the compiler where we manipulate ranges/anti-ranges directly, >>>> all of which should be adapted in the future. Everywhere there is a >>>> mention of VR_RANGE/VR_ANTI_RANGE in the compiler is suspect. We should >>>> ideally be using intersect/union/may_contain_p/null_p, etc. >>> >>> null_p is a bad name btw, I just confused it with empty_p ... (which we have >>> as undefined_p). contains_only_zero_p would be less confusing. >> >> Yes, a horrible name. I noticed so as I debugged precisely this bit. >> How about zero_p? > > Probably the same ambiguous connotation? But yes, way better than null_p. Well...for starters I started with the nomenclature already in VRP which was range_is_null. Also, how is zero_p() ambiguous? We want to know if the range is [0, 0]. That reads pretty obvious to me. Aldy
On 11/8/18 7:44 AM, Aldy Hernandez wrote: > > > On 11/8/18 9:41 AM, Richard Biener wrote: >> On Thu, Nov 8, 2018 at 3:05 PM Aldy Hernandez <aldyh@redhat.com> wrote: >>> >>> >>> >>> On 11/8/18 8:59 AM, Richard Biener wrote: >>>> On Thu, Nov 8, 2018 at 12:52 PM Aldy Hernandez <aldyh@redhat.com> >>>> wrote: >>>>> >>>>> get/set_range_info() currently returns the extremes of a range. I >>>>> have >>>>> implemented overloaded variants that return a proper range. In the >>>>> future we should use actual ranges throughout, and not depend on range >>>>> extremes, as depending on this behavior could causes us to lose >>>>> precision. >>>>> >>>>> I am also including changes to size_must_be_zero_p() to show how we >>>>> should be using the range API, as opposed to performing error prone >>>>> ad-hoc calculations on ranges and anti-ranges. >>>> >>>> Yeah, I've been talking this all along but not being heard... >>> >>> My girlfriend says I don't listen. It could be related. >>> >>>>> Martin, I'm not saying your code is wrong. There are numerous other >>>>> places in the compiler where we manipulate ranges/anti-ranges >>>>> directly, >>>>> all of which should be adapted in the future. Everywhere there is a >>>>> mention of VR_RANGE/VR_ANTI_RANGE in the compiler is suspect. We >>>>> should >>>>> ideally be using intersect/union/may_contain_p/null_p, etc. >>>> >>>> null_p is a bad name btw, I just confused it with empty_p ... (which >>>> we have >>>> as undefined_p). contains_only_zero_p would be less confusing. >>> >>> Yes, a horrible name. I noticed so as I debugged precisely this bit. >>> How about zero_p? >> >> Probably the same ambiguous connotation? But yes, way better than >> null_p. > > Well...for starters I started with the nomenclature already in VRP which > was range_is_null. Probably due to using it for NULL pointer test elimination. But yea, in retrospect it's an awful name. > > Also, how is zero_p() ambiguous? We want to know if the range is [0, > 0]. That reads pretty obvious to me. Seems reasonable to me. THe question is will we push that name into all the other places that use "null" when discussing ranges like ~[0,0] or ranges that ultimately include [0,0]. Not all of these are in [E]VRP IIRC. jeff
On 11/08/2018 04:52 AM, Aldy Hernandez wrote: > get/set_range_info() currently returns the extremes of a range. I have > implemented overloaded variants that return a proper range. In the > future we should use actual ranges throughout, and not depend on range > extremes, as depending on this behavior could causes us to lose precision. > > I am also including changes to size_must_be_zero_p() to show how we > should be using the range API, as opposed to performing error prone > ad-hoc calculations on ranges and anti-ranges. > > Martin, I'm not saying your code is wrong. There are numerous other > places in the compiler where we manipulate ranges/anti-ranges directly, > all of which should be adapted in the future. Everywhere there is a > mention of VR_RANGE/VR_ANTI_RANGE in the compiler is suspect. We should > ideally be using intersect/union/may_contain_p/null_p, etc. > > OK pending another round of tests? (As I had tested this patch along > with a whole slew of other upcoming changes ;-)). I'm all for simplification! :) I'd love it if the API made it possible to write these expressions using native operators and intuitive names, without having to worry about details like what types they are represented in or how to convert between them. It would be great if a function like size_must_be_zero_p could be coded along the lines similar to: return intersect (size, value_range (ssize_type_node) - 0) == 0; It shouldn't have to twiddle bits to compute the wide_int value of SSIZE_MAX or wrestle with convertibility warts by calling build_int_cst and and wide_int_to_tree. Martin PS I think we should be able to get close to the syntax above with a few value_range ctors: value_range::value_range (tree type_or_expression); template <class IntegerType> value_range::value_range (const IntegerType&); a non-member minus operator to compute the difference: value_range operator- (const value_range&, const value_range&); a non-member intersect function like this: value_range intersect (const value_range&, const value_range&); and a non-member equality operator: bool operator== (const value_range&, const value_range&); With the above, because a tree and any integer (including wide_int, offset_int, and all the other flavors du jour) implicitly convert to a value_range (possibly representing a single value), the subtraction, intersection, and equality would apply to any and all of those types as well without callers having to spell out or even know what representation they're dealing with.
On 11/8/18 4:47 PM, Martin Sebor wrote: > On 11/08/2018 04:52 AM, Aldy Hernandez wrote: >> get/set_range_info() currently returns the extremes of a range. I have >> implemented overloaded variants that return a proper range. In the >> future we should use actual ranges throughout, and not depend on range >> extremes, as depending on this behavior could causes us to lose >> precision. >> >> I am also including changes to size_must_be_zero_p() to show how we >> should be using the range API, as opposed to performing error prone >> ad-hoc calculations on ranges and anti-ranges. >> >> Martin, I'm not saying your code is wrong. There are numerous other >> places in the compiler where we manipulate ranges/anti-ranges directly, >> all of which should be adapted in the future. Everywhere there is a >> mention of VR_RANGE/VR_ANTI_RANGE in the compiler is suspect. We should >> ideally be using intersect/union/may_contain_p/null_p, etc. >> >> OK pending another round of tests? (As I had tested this patch along >> with a whole slew of other upcoming changes ;-)). > > I'm all for simplification! :) > > I'd love it if the API made it possible to write these expressions > using native operators and intuitive names, without having to worry > about details like what types they are represented in or how to > convert between them. It would be great if a function like > size_must_be_zero_p could be coded along the lines similar to: > > return intersect (size, value_range (ssize_type_node) - 0) == 0; > > It shouldn't have to twiddle bits to compute the wide_int value > of SSIZE_MAX or wrestle with convertibility warts by calling > build_int_cst and and wide_int_to_tree. > > Martin > > PS I think we should be able to get close to the syntax above > with a few value_range ctors: > > value_range::value_range (tree type_or_expression); Would this create the range for [type, type] or [-MIN, type] or what? > > template <class IntegerType> > value_range::value_range (const IntegerType&); > > a non-member minus operator to compute the difference: > > value_range operator- (const value_range&, const value_range&); > > a non-member intersect function like this: > > value_range intersect (const value_range&, const value_range&); > > and a non-member equality operator: > > bool operator== (const value_range&, const value_range&); > > With the above, because a tree and any integer (including wide_int, > offset_int, and all the other flavors du jour) implicitly convert > to a value_range (possibly representing a single value), > the subtraction, intersection, and equality would apply to any and > all of those types as well without callers having to spell out or > even know what representation they're dealing with. I like it. Aldy
On 11/09/2018 03:02 AM, Aldy Hernandez wrote: > > > On 11/8/18 4:47 PM, Martin Sebor wrote: >> On 11/08/2018 04:52 AM, Aldy Hernandez wrote: >>> get/set_range_info() currently returns the extremes of a range. I have >>> implemented overloaded variants that return a proper range. In the >>> future we should use actual ranges throughout, and not depend on range >>> extremes, as depending on this behavior could causes us to lose >>> precision. >>> >>> I am also including changes to size_must_be_zero_p() to show how we >>> should be using the range API, as opposed to performing error prone >>> ad-hoc calculations on ranges and anti-ranges. >>> >>> Martin, I'm not saying your code is wrong. There are numerous other >>> places in the compiler where we manipulate ranges/anti-ranges directly, >>> all of which should be adapted in the future. Everywhere there is a >>> mention of VR_RANGE/VR_ANTI_RANGE in the compiler is suspect. We should >>> ideally be using intersect/union/may_contain_p/null_p, etc. >>> >>> OK pending another round of tests? (As I had tested this patch along >>> with a whole slew of other upcoming changes ;-)). >> >> I'm all for simplification! :) >> >> I'd love it if the API made it possible to write these expressions >> using native operators and intuitive names, without having to worry >> about details like what types they are represented in or how to >> convert between them. It would be great if a function like >> size_must_be_zero_p could be coded along the lines similar to: >> >> return intersect (size, value_range (ssize_type_node) - 0) == 0; >> >> It shouldn't have to twiddle bits to compute the wide_int value >> of SSIZE_MAX or wrestle with convertibility warts by calling >> build_int_cst and and wide_int_to_tree. >> >> Martin >> >> PS I think we should be able to get close to the syntax above >> with a few value_range ctors: >> >> value_range::value_range (tree type_or_expression); > > Would this create the range for [type, type] or [-MIN, type] or what? For a TYPE it would create [TYPE_MIN, TYPE_MAX]. >> template <class IntegerType> >> value_range::value_range (const IntegerType&); >> >> a non-member minus operator to compute the difference: >> >> value_range operator- (const value_range&, const value_range&); >> >> a non-member intersect function like this: >> >> value_range intersect (const value_range&, const value_range&); >> >> and a non-member equality operator: >> >> bool operator== (const value_range&, const value_range&); >> >> With the above, because a tree and any integer (including wide_int, >> offset_int, and all the other flavors du jour) implicitly convert >> to a value_range (possibly representing a single value), >> the subtraction, intersection, and equality would apply to any and >> all of those types as well without callers having to spell out or >> even know what representation they're dealing with. > > I like it. I'm glad! When can we get it? ;-) Seriously, if this is something we would like to see I'm happy to help put it together. Martin
gcc/ * gimple-fold.c (size_must_be_zero_p): Use value_range API instead of performing ad-hoc calculations. * tree-ssanames.c (set_range_info): New overloaded function accepting value_range &. (get_range_info): Same. * tree-ssanames.h (set_range_info_raw): Remove. (set_range_info): New prototype. (get_range_info): Same. diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c index 5468b604dec..4d9bdcd097f 100644 --- a/gcc/gimple-fold.c +++ b/gcc/gimple-fold.c @@ -635,9 +635,8 @@ var_decl_component_p (tree var) && TREE_CODE (TREE_OPERAND (inner, 0)) == ADDR_EXPR)); } -/* If the SIZE argument representing the size of an object is in a range - of values of which exactly one is valid (and that is zero), return - true, otherwise false. */ +/* Return TRUE if the SIZE argument, representing the size of an + object, is in a range of values of which exactly zero is valid. */ static bool size_must_be_zero_p (tree size) @@ -648,21 +647,19 @@ size_must_be_zero_p (tree size) if (TREE_CODE (size) != SSA_NAME || !INTEGRAL_TYPE_P (TREE_TYPE (size))) return false; - wide_int min, max; - enum value_range_kind rtype = get_range_info (size, &min, &max); - if (rtype != VR_ANTI_RANGE) - return false; - tree type = TREE_TYPE (size); int prec = TYPE_PRECISION (type); - wide_int wone = wi::one (prec); - /* Compute the value of SSIZE_MAX, the largest positive value that can be stored in ssize_t, the signed counterpart of size_t. */ wide_int ssize_max = wi::lshift (wi::one (prec), prec - 1) - 1; - - return wi::eq_p (min, wone) && wi::geu_p (max, ssize_max); + value_range valid_range (VR_RANGE, + build_int_cst (type, 0), + wide_int_to_tree (type, ssize_max)); + value_range vr; + get_range_info (size, vr); + vr.intersect (&valid_range); + return vr.null_p (); } /* Fold function call to builtin mem{{,p}cpy,move}. Try to detect and diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c index ff906e831e5..a2c2efb634a 100644 --- a/gcc/tree-ssanames.c +++ b/gcc/tree-ssanames.c @@ -398,6 +398,15 @@ set_range_info (tree name, enum value_range_kind range_type, set_range_info_raw (name, range_type, min, max); } +/* Store range information for NAME from a value_range. */ + +void +set_range_info (tree name, const value_range &vr) +{ + wide_int min = wi::to_wide (vr.min ()); + wide_int max = wi::to_wide (vr.max ()); + set_range_info (name, vr.kind (), min, max); +} /* Gets range information MIN, MAX and returns enum value_range_kind corresponding to tree ssa_name NAME. enum value_range_kind returned @@ -421,6 +430,27 @@ get_range_info (const_tree name, wide_int *min, wide_int *max) return SSA_NAME_RANGE_TYPE (name); } +/* Gets range information corresponding to ssa_name NAME and stores it + in a value_range VR. Returns the value_range_kind. */ + +enum value_range_kind +get_range_info (const_tree name, value_range &vr) +{ + tree min, max; + wide_int wmin, wmax; + enum value_range_kind kind = get_range_info (name, &wmin, &wmax); + + if (kind == VR_VARYING || kind == VR_UNDEFINED) + min = max = NULL; + else + { + min = wide_int_to_tree (TREE_TYPE (name), wmin); + max = wide_int_to_tree (TREE_TYPE (name), wmax); + } + vr = value_range (kind, min, max); + return kind; +} + /* Set nonnull attribute to pointer NAME. */ void diff --git a/gcc/tree-ssanames.h b/gcc/tree-ssanames.h index 18a001a5461..a5ff14e524f 100644 --- a/gcc/tree-ssanames.h +++ b/gcc/tree-ssanames.h @@ -69,12 +69,11 @@ struct GTY ((variable_size)) range_info_def { /* Sets the value range to SSA. */ extern void set_range_info (tree, enum value_range_kind, const wide_int_ref &, const wide_int_ref &); -extern void set_range_info_raw (tree, enum value_range_kind, - const wide_int_ref &, - const wide_int_ref &); +extern void set_range_info (tree, const value_range &); /* Gets the value range from SSA. */ extern enum value_range_kind get_range_info (const_tree, wide_int *, wide_int *); +extern enum value_range_kind get_range_info (const_tree, value_range &); extern void set_nonzero_bits (tree, const wide_int_ref &); extern wide_int get_nonzero_bits (const_tree); extern bool ssa_name_has_boolean_range (tree);