Message ID | 20220922164911.2566143-1-aldyh@redhat.com |
---|---|
State | New |
Headers | show |
Series | frange: dump hex values when dumping FP numbers. | expand |
If it's not too cumbersome, I suggest dumping both. In my neck-of-the-woods (meteorology) I have seen this done just to ensure that algorithms that are supposed to be bit-reproducable actually are - and that it can be checked visually. Kind regards, Toon. On 9/22/22 18:49, Aldy Hernandez via Gcc-patches wrote: > It has been suggested that if we start bumping numbers by an ULP when > calculating open ranges (for example the numbers less than 3.0) that > dumping these will become increasingly harder to read, and instead we > should opt for the hex representation. I still find the floating > point representation easier to read for most numbers, but perhaps we > could have both? > > With this patch this is the representation for [15.0, 20.0]: > > [frange] float [1.5e+1 (0x0.fp+4), 2.0e+1 (0x0.ap+5)] > > Would you find this useful, or should we stick to the hex > representation only (or something altogether different)? > > Tested on x86-64 Linux. > > gcc/ChangeLog: > > * value-range-pretty-print.cc (vrange_printer::print_real_value): New. > (vrange_printer::visit): Call print_real_value. > * value-range-pretty-print.h: New print_real_value. > --- > gcc/value-range-pretty-print.cc | 16 ++++++++++++---- > gcc/value-range-pretty-print.h | 1 + > 2 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/gcc/value-range-pretty-print.cc b/gcc/value-range-pretty-print.cc > index eb7442229ba..51be037c254 100644 > --- a/gcc/value-range-pretty-print.cc > +++ b/gcc/value-range-pretty-print.cc > @@ -117,6 +117,16 @@ vrange_printer::print_irange_bitmasks (const irange &r) const > pp_string (pp, buf); > } > > +void > +vrange_printer::print_real_value (tree type, const REAL_VALUE_TYPE &r) const > +{ > + char s[60]; > + tree t = build_real (type, r); > + dump_generic_node (pp, t, 0, TDF_NONE, false); > + real_to_hexadecimal (s, &r, sizeof (s), 0, 1); > + pp_printf (pp, " (%s)", s); > +} > + > // Print an frange. > > void > @@ -141,11 +151,9 @@ vrange_printer::visit (const frange &r) const > bool has_endpoints = !r.known_isnan (); > if (has_endpoints) > { > - dump_generic_node (pp, > - build_real (type, r.lower_bound ()), 0, TDF_NONE, false); > + print_real_value (type, r.lower_bound ()); > pp_string (pp, ", "); > - dump_generic_node (pp, > - build_real (type, r.upper_bound ()), 0, TDF_NONE, false); > + print_real_value (type, r.upper_bound ()); > } > pp_character (pp, ']'); > print_frange_nan (r); > diff --git a/gcc/value-range-pretty-print.h b/gcc/value-range-pretty-print.h > index 20c26598fe7..a9ae5a7b4cc 100644 > --- a/gcc/value-range-pretty-print.h > +++ b/gcc/value-range-pretty-print.h > @@ -32,6 +32,7 @@ private: > void print_irange_bound (const wide_int &w, tree type) const; > void print_irange_bitmasks (const irange &) const; > void print_frange_nan (const frange &) const; > + void print_real_value (tree type, const REAL_VALUE_TYPE &r) const; > > pretty_printer *pp; > };
On 9/22/22 10:49, Aldy Hernandez via Gcc-patches wrote: > It has been suggested that if we start bumping numbers by an ULP when > calculating open ranges (for example the numbers less than 3.0) that > dumping these will become increasingly harder to read, and instead we > should opt for the hex representation. I still find the floating > point representation easier to read for most numbers, but perhaps we > could have both? > > With this patch this is the representation for [15.0, 20.0]: > > [frange] float [1.5e+1 (0x0.fp+4), 2.0e+1 (0x0.ap+5)] > > Would you find this useful, or should we stick to the hex > representation only (or something altogether different)? > > Tested on x86-64 Linux. > > gcc/ChangeLog: > > * value-range-pretty-print.cc (vrange_printer::print_real_value): New. > (vrange_printer::visit): Call print_real_value. > * value-range-pretty-print.h: New print_real_value. The big advantage of the hex representation is you can feed that back into the compiler trivially and be confident the bit pattern hasn't changed. I've found it invaluable when doing deep FP analysis. jeff
On Thu, Sep 22, 2022 at 06:49:10PM +0200, Aldy Hernandez wrote: > It has been suggested that if we start bumping numbers by an ULP when > calculating open ranges (for example the numbers less than 3.0) that > dumping these will become increasingly harder to read, and instead we > should opt for the hex representation. I still find the floating > point representation easier to read for most numbers, but perhaps we > could have both? > > With this patch this is the representation for [15.0, 20.0]: > > [frange] float [1.5e+1 (0x0.fp+4), 2.0e+1 (0x0.ap+5)] > > Would you find this useful, or should we stick to the hex > representation only (or something altogether different)? I think dumping both is the way to go, but real_to_hexadecimal doesn't do anything useful with decimal floats, so that part should be guarded on !DECIMAL_FLOAT_TYPE_P (type). Why do you build a tree + dump_generic_node for decimal instead of real_to_decimal_for_mode ? The former I think calls: char string[100]; real_to_decimal (string, &d, sizeof (string), 0, 1); so perhaps: char s[100]; real_to_decimal_for_mode (s, &r, sizeof (string), 0, 1, TYPE_MODE (type)); pp_string (pp, "%s", s); if (!DECIMAL_FLOAT_TYPE_P (type)) { real_to_hexadecimal (s, &r, sizeof (s), 0, 1); pp_printf (pp, " (%s)", s); } ? Jakub
On Thu, Sep 22, 2022 at 11:04 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Sep 22, 2022 at 06:49:10PM +0200, Aldy Hernandez wrote: > > It has been suggested that if we start bumping numbers by an ULP when > > calculating open ranges (for example the numbers less than 3.0) that > > dumping these will become increasingly harder to read, and instead we > > should opt for the hex representation. I still find the floating > > point representation easier to read for most numbers, but perhaps we > > could have both? > > > > With this patch this is the representation for [15.0, 20.0]: > > > > [frange] float [1.5e+1 (0x0.fp+4), 2.0e+1 (0x0.ap+5)] > > > > Would you find this useful, or should we stick to the hex > > representation only (or something altogether different)? > > I think dumping both is the way to go, but real_to_hexadecimal doesn't > do anything useful with decimal floats, so that part should be > guarded on !DECIMAL_FLOAT_TYPE_P (type). Remember that decimal floats are not supported for frange, but I suppose it's good form to guard the print for when we do. > > Why do you build a tree + dump_generic_node for decimal instead of > real_to_decimal_for_mode ? Honestly, cause I was too lazy. That code is inherited from irange and I figured it's just a dumping routine, but I'm happy to fix it. > The former I think calls: > char string[100]; > real_to_decimal (string, &d, sizeof (string), 0, 1); > so perhaps: > char s[100]; > real_to_decimal_for_mode (s, &r, sizeof (string), 0, 1, TYPE_MODE (type)); > pp_string (pp, "%s", s); > if (!DECIMAL_FLOAT_TYPE_P (type)) > { > real_to_hexadecimal (s, &r, sizeof (s), 0, 1); > pp_printf (pp, " (%s)", s); > } > ? Thanks. I'm retesting the following and will commit if it succeeds since we seem to have overwhelming consensus :). Aldy
diff --git a/gcc/value-range-pretty-print.cc b/gcc/value-range-pretty-print.cc index eb7442229ba..51be037c254 100644 --- a/gcc/value-range-pretty-print.cc +++ b/gcc/value-range-pretty-print.cc @@ -117,6 +117,16 @@ vrange_printer::print_irange_bitmasks (const irange &r) const pp_string (pp, buf); } +void +vrange_printer::print_real_value (tree type, const REAL_VALUE_TYPE &r) const +{ + char s[60]; + tree t = build_real (type, r); + dump_generic_node (pp, t, 0, TDF_NONE, false); + real_to_hexadecimal (s, &r, sizeof (s), 0, 1); + pp_printf (pp, " (%s)", s); +} + // Print an frange. void @@ -141,11 +151,9 @@ vrange_printer::visit (const frange &r) const bool has_endpoints = !r.known_isnan (); if (has_endpoints) { - dump_generic_node (pp, - build_real (type, r.lower_bound ()), 0, TDF_NONE, false); + print_real_value (type, r.lower_bound ()); pp_string (pp, ", "); - dump_generic_node (pp, - build_real (type, r.upper_bound ()), 0, TDF_NONE, false); + print_real_value (type, r.upper_bound ()); } pp_character (pp, ']'); print_frange_nan (r); diff --git a/gcc/value-range-pretty-print.h b/gcc/value-range-pretty-print.h index 20c26598fe7..a9ae5a7b4cc 100644 --- a/gcc/value-range-pretty-print.h +++ b/gcc/value-range-pretty-print.h @@ -32,6 +32,7 @@ private: void print_irange_bound (const wide_int &w, tree type) const; void print_irange_bitmasks (const irange &) const; void print_frange_nan (const frange &) const; + void print_real_value (tree type, const REAL_VALUE_TYPE &r) const; pretty_printer *pp; };