| Message ID | 20201109144322.616847-1-aldyh@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Cleanup irange::set. | expand |
Hi, On Mon, 9 Nov 2020 at 15:43, Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > [This is actually part of a larger patch that actually changes > behavior, but I thought I'd commit the non-invasive cleanups first > which will simplify the upcoming work.] > > irange::set was doing more work than it should for legacy ranges. > I cleaned up various unnecessary calls to swap_out_of_order_endpoints, > as well as some duplicate code that could be done with normalize_min_max. > > I also removed an obsolete comment wrt sticky infinite overflows. > Not only did the -INF/+INF(OVF) code get removed in 2017, > but normalize_min_max() uses wide ints, which ignored overflows > altogether. > > Pushed. > > gcc/ChangeLog: > > * value-range.cc (irange::swap_out_of_order_endpoints): Rewrite > into static function. > (irange::set): Cleanup redundant manipulations. > * value-range.h (irange::normalize_min_max): Modify object > in-place instead of modifying arguments. This is causing regressions on aarch64, such as: FAIL: gcc.target/aarch64/pr88838.c (internal compiler error) FAIL: gcc.target/aarch64/pr88838.c (test for excess errors) Excess errors: during GIMPLE pass: dom /gcc/testsuite/gcc.target/aarch64/pr88838.c:5:1: internal compiler error: tree check: expected integer_cst, have poly_int_cst in to_wide, at tree.h:5950 0x66741d tree_check_failed(tree_node const*, char const*, int, char const*, ...) /gcc/tree.c:9754 0x6404e8 tree_check /gcc/tree.h:3570 0x6404e8 to_wide /gcc/tree.h:5950 0x7f0816 wi::to_wide(tree_node const*) /gcc/tree.h:3505 0x1118bdc irange::legacy_upper_bound(unsigned int) const /gcc/value-range.cc:447 0x1122cd4 irange::upper_bound(unsigned int) const /gcc/value-range.h:510 0x19493be range_operator::fold_range(irange&, tree_node*, irange const&, irange const&) const /gcc/range-op.cc:159 0x19494f4 range_operator::fold_range(irange&, tree_node*, irange const&, irange const&) const /gcc/range-op.cc:74 0x10cfee6 range_fold_binary_expr(int_range<1u>*, tree_code, tree_node*, int_range<1u> const*, int_range<1u> const*) /gcc/tree-vrp.c:1243 0x116fda4 vr_values::extract_range_from_binary_expr(value_range_equiv*, tree_code, tree_node*, tree_node*, tree_node*) /gcc/vr-values.c:853 0x1178b3c vr_values::extract_range_from_assignment(value_range_equiv*, gassign*) /gcc/vr-values.c:1564 0x185bcbe evrp_range_analyzer::record_ranges_from_stmt(gimple*, bool) /gcc/gimple-ssa-evrp-analyze.c:304 0xee7f9b dom_opt_dom_walker::before_dom_children(basic_block_def*) /gcc/tree-ssa-dom.c:1458 0x1817487 dom_walker::walk(basic_block_def*) /gcc/domwalk.c:309 0xee4f8b execute /gcc/tree-ssa-dom.c:724 (actually I can see 3245 ICEs on aarch64) Can you fix it? Thanks > --- > gcc/value-range.cc | 70 ++++++++++++++++------------------------------ > gcc/value-range.h | 28 +++++++++---------- > 2 files changed, 37 insertions(+), 61 deletions(-) > > diff --git a/gcc/value-range.cc b/gcc/value-range.cc > index 5827e812216..2124e229e0c 100644 > --- a/gcc/value-range.cc > +++ b/gcc/value-range.cc > @@ -131,13 +131,14 @@ irange::copy_to_legacy (const irange &src) > set (src.tree_lower_bound (), src.tree_upper_bound ()); > } > > -// Swap min/max if they are out of order. Return TRUE if further > -// processing of the range is necessary, FALSE otherwise. > +// Swap MIN/MAX if they are out of order and adjust KIND appropriately. > > -bool > -irange::swap_out_of_order_endpoints (tree &min, tree &max, > - value_range_kind &kind) > +static void > +swap_out_of_order_endpoints (tree &min, tree &max, value_range_kind &kind) > { > + gcc_checking_assert (kind != VR_UNDEFINED); > + if (kind == VR_VARYING) > + return; > /* Wrong order for min and max, to swap them and the VR type we need > to adjust them. */ > if (tree_int_cst_lt (max, min)) > @@ -149,8 +150,8 @@ irange::swap_out_of_order_endpoints (tree &min, tree &max, > for VR_ANTI_RANGE empty range, so drop to varying as well. */ > if (TYPE_PRECISION (TREE_TYPE (min)) == 1) > { > - set_varying (TREE_TYPE (min)); > - return false; > + kind = VR_VARYING; > + return; > } > > one = build_int_cst (TREE_TYPE (min), 1); > @@ -163,12 +164,11 @@ irange::swap_out_of_order_endpoints (tree &min, tree &max, > to varying in this case. */ > if (tree_int_cst_lt (max, min)) > { > - set_varying (TREE_TYPE (min)); > - return false; > + kind = VR_VARYING; > + return; > } > kind = kind == VR_RANGE ? VR_ANTI_RANGE : VR_RANGE; > } > - return true; > } > > void > @@ -253,13 +253,6 @@ irange::set (tree min, tree max, value_range_kind kind) > && (POLY_INT_CST_P (min) || POLY_INT_CST_P (max))) > kind = VR_VARYING; > > - if (kind == VR_VARYING) > - { > - set_varying (TREE_TYPE (min)); > - return; > - } > - > - tree type = TREE_TYPE (min); > // Nothing to canonicalize for symbolic ranges. > if (TREE_CODE (min) != INTEGER_CST > || TREE_CODE (max) != INTEGER_CST) > @@ -270,8 +263,13 @@ irange::set (tree min, tree max, value_range_kind kind) > m_num_ranges = 1; > return; > } > - if (!swap_out_of_order_endpoints (min, max, kind)) > - goto cleanup_set; > + > + swap_out_of_order_endpoints (min, max, kind); > + if (kind == VR_VARYING) > + { > + set_varying (TREE_TYPE (min)); > + return; > + } > > // Anti-ranges that can be represented as ranges should be so. > if (kind == VR_ANTI_RANGE) > @@ -280,6 +278,7 @@ irange::set (tree min, tree max, value_range_kind kind) > values < -INF and values > INF as -INF/INF as well. */ > bool is_min = vrp_val_is_min (min); > bool is_max = vrp_val_is_max (max); > + tree type = TREE_TYPE (min); > > if (is_min && is_max) > { > @@ -314,38 +313,17 @@ irange::set (tree min, tree max, value_range_kind kind) > kind = VR_RANGE; > } > } > - else if (!swap_out_of_order_endpoints (min, max, kind)) > - goto cleanup_set; > - > - /* Do not drop [-INF(OVF), +INF(OVF)] to varying. (OVF) has to be sticky > - to make sure VRP iteration terminates, otherwise we can get into > - oscillations. */ > - if (!normalize_min_max (type, min, max, kind)) > - { > - m_kind = kind; > - m_base[0] = min; > - m_base[1] = max; > - m_num_ranges = 1; > - if (flag_checking) > - verify_range (); > - } > > - cleanup_set: > - // Avoid using TYPE_{MIN,MAX}_VALUE because -fstrict-enums can > - // restrict those to a subset of what actually fits in the type. > - // Instead use the extremes of the type precision > - unsigned prec = TYPE_PRECISION (type); > - signop sign = TYPE_SIGN (type); > - if (wi::eq_p (wi::to_wide (min), wi::min_value (prec, sign)) > - && wi::eq_p (wi::to_wide (max), wi::max_value (prec, sign))) > - m_kind = VR_VARYING; > - else if (undefined_p ()) > - m_kind = VR_UNDEFINED; > + m_kind = kind; > + m_base[0] = min; > + m_base[1] = max; > + m_num_ranges = 1; > + normalize_min_max (); > if (flag_checking) > verify_range (); > } > > -/* Check the validity of the range. */ > +// Check the validity of the range. > > void > irange::verify_range () > diff --git a/gcc/value-range.h b/gcc/value-range.h > index 760ee772316..a483fc802dd 100644 > --- a/gcc/value-range.h > +++ b/gcc/value-range.h > @@ -111,8 +111,7 @@ protected: > void irange_set (tree, tree); > void irange_set_anti_range (tree, tree); > > - bool swap_out_of_order_endpoints (tree &min, tree &max, value_range_kind &); > - bool normalize_min_max (tree type, tree min, tree max, value_range_kind); > + void normalize_min_max (); > > bool legacy_mode_p () const; > bool legacy_equal_p (const irange &) const; > @@ -566,7 +565,7 @@ irange::set_zero (tree type) > irange_set (z, z); > } > > -// Normalize [MIN, MAX] into VARYING and ~[MIN, MAX] into UNDEFINED. > +// Normalize a range to VARYING or UNDEFINED if possible. > // > // Avoid using TYPE_{MIN,MAX}_VALUE because -fstrict-enums can > // restrict those to a subset of what actually fits in the type. > @@ -575,24 +574,23 @@ irange::set_zero (tree type) > // whereas if we used TYPE_*_VAL, said function would just punt upon > // seeing a VARYING. > > -inline bool > -irange::normalize_min_max (tree type, tree min, tree max, > - value_range_kind kind) > +inline void > +irange::normalize_min_max () > { > - unsigned prec = TYPE_PRECISION (type); > - signop sign = TYPE_SIGN (type); > - if (wi::eq_p (wi::to_wide (min), wi::min_value (prec, sign)) > - && wi::eq_p (wi::to_wide (max), wi::max_value (prec, sign))) > + gcc_checking_assert (legacy_mode_p ()); > + gcc_checking_assert (!undefined_p ()); > + unsigned prec = TYPE_PRECISION (type ()); > + signop sign = TYPE_SIGN (type ()); > + if (wi::eq_p (wi::to_wide (min ()), wi::min_value (prec, sign)) > + && wi::eq_p (wi::to_wide (max ()), wi::max_value (prec, sign))) > { > - if (kind == VR_RANGE) > - set_varying (type); > - else if (kind == VR_ANTI_RANGE) > + if (m_kind == VR_RANGE) > + set_varying (type ()); > + else if (m_kind == VR_ANTI_RANGE) > set_undefined (); > else > gcc_unreachable (); > - return true; > } > - return false; > } > > // Return the maximum value for TYPE. > -- > 2.26.2 >
This breaks aarch64.
spawn -ignore SIGHUP /opt/gcc/test/Build/gcc/xgcc -B/opt/gcc/test/Build/gcc/ -mabi=lp64 -fdiagnostics-plain-output -march=armv8.2-a+sve -O3 --save-temps -ffat-lto-objects -fno-ident -c -o abs_1.o /opt/gcc/test/gcc/testsuite/gcc.target/aarch64/sve/abs_1.c
during GIMPLE pass: dom
/opt/gcc/test/gcc/testsuite/gcc.target/aarch64/sve/abs_1.c: In function 'vneg_int8_t':
/opt/gcc/test/gcc/testsuite/gcc.target/aarch64/sve/abs_1.c:13:6: internal compiler error: tree check: expected integer_cst, have poly_int_cst in to_wide, at tree.h:5950
0x6743ab tree_check_failed(tree_node const*, char const*, int, char const*, ...)
../../gcc/tree.c:9752
0x674f3b tree_int_cst_elt_check(tree_node const*, int, char const*, int, char const*)
../../gcc/tree.h:3502
0x1033d67 tree_int_cst_elt_check(tree_node const*, int, char const*, int, char const*)
../../gcc/tree.h:3437
0x1033d67 wi::to_wide(tree_node const*)
../../gcc/tree.h:5951
0x1033d67 irange::legacy_lower_bound(unsigned int) const
../../gcc/value-range.cc:420
0x103ad23 irange::lower_bound(unsigned int) const
../../gcc/value-range.h:497
0x183154f range_operator::fold_range(irange&, tree_node*, irange const&, irange const&) const
../../gcc/range-op.cc:159
0xfeae4b range_fold_binary_expr(int_range<1u>*, tree_code, tree_node*, int_range<1u> const*, int_range<1u> const*)
../../gcc/tree-vrp.c:1243
0x107dcaf vr_values::extract_range_from_binary_expr(value_range_equiv*, tree_code, tree_node*, tree_node*, tree_node*)
../../gcc/vr-values.c:853
0x1086917 vr_values::extract_range_from_assignment(value_range_equiv*, gassign*)
../../gcc/vr-values.c:1561
0x1747eb3 evrp_range_analyzer::record_ranges_from_stmt(gimple*, bool)
../../gcc/gimple-ssa-evrp-analyze.c:304
0xe3b047 dom_opt_dom_walker::before_dom_children(basic_block_def*)
../../gcc/tree-ssa-dom.c:1458
0x170cf7f dom_walker::walk(basic_block_def*)
../../gcc/domwalk.c:309
0xe38b03 execute
../../gcc/tree-ssa-dom.c:724
Andreas.
> (actually I can see 3245 ICEs on aarch64) > > Can you fix it? Sure can. Richard, I seem to have incorrectly removed the early exit for varying, and that affected the changes you made for poly ints. Is there any reason we can't just exit and set varying without checking for kind != VR_VARYING? The attached patch fixes the problem for aarch64. Testing in progress... Aldy Early exit from irange::set for poly ints. My previous cleanups to irange::set moved the early exit when VARYING. This caused poly int varyings to be created with incorrect min/max. We can just set varying and exit for all poly ints. gcc/ChangeLog: * value-range.cc (irange::set): Early exit for poly ints. diff --git a/gcc/value-range.cc b/gcc/value-range.cc index 61f7da278d6..178ef1b0a4f 100644 --- a/gcc/value-range.cc +++ b/gcc/value-range.cc @@ -249,9 +249,11 @@ irange::set (tree min, tree max, value_range_kind kind) return; } - if (kind != VR_VARYING - && (POLY_INT_CST_P (min) || POLY_INT_CST_P (max))) - kind = VR_VARYING; + if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max)) + { + set_varying (TREE_TYPE (min)); + return; + } // Nothing to canonicalize for symbolic ranges. if (TREE_CODE (min) != INTEGER_CST
Aldy Hernandez <aldyh@redhat.com> writes: >> (actually I can see 3245 ICEs on aarch64) >> >> Can you fix it? > > Sure can. > > Richard, I seem to have incorrectly removed the early exit for varying, > and that affected the changes you made for poly ints. Is there any > reason we can't just exit and set varying without checking for kind != > VR_VARYING? No reason, it seemed more natural to drop to a lower kind with the old code :-) (But not with the new code.) But it isn't obvious to me why the code is now structed the way it is. if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max)) { set_varying (TREE_TYPE (min)); return; } // Nothing to canonicalize for symbolic ranges. if (TREE_CODE (min) != INTEGER_CST || TREE_CODE (max) != INTEGER_CST) { m_kind = kind; m_base[0] = min; m_base[1] = max; m_num_ranges = 1; return; } swap_out_of_order_endpoints (min, max, kind); if (kind == VR_VARYING) { set_varying (TREE_TYPE (min)); return; } Why do we want to check “min” and “max” being INTEGER_CST before “kind” being VR_VARYING, and the potentially record VR_VARYING with specific bounds? And why do we want to swap the “min” and “max” before checking whether “kind” is VR_VARYING (when we'll then drop the min and max anyway)? I think this would benefit from a bit more commentary at least. Thanks, Richard > > The attached patch fixes the problem for aarch64. > > Testing in progress... > Aldy > > Early exit from irange::set for poly ints. > > My previous cleanups to irange::set moved the early exit when > VARYING. This caused poly int varyings to be created with > incorrect min/max. > > We can just set varying and exit for all poly ints. > > gcc/ChangeLog: > > * value-range.cc (irange::set): Early exit for poly ints. > > diff --git a/gcc/value-range.cc b/gcc/value-range.cc > index 61f7da278d6..178ef1b0a4f 100644 > --- a/gcc/value-range.cc > +++ b/gcc/value-range.cc > @@ -249,9 +249,11 @@ irange::set (tree min, tree max, value_range_kind kind) > return; > } > > - if (kind != VR_VARYING > - && (POLY_INT_CST_P (min) || POLY_INT_CST_P (max))) > - kind = VR_VARYING; > + if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max)) > + { > + set_varying (TREE_TYPE (min)); > + return; > + } > > // Nothing to canonicalize for symbolic ranges. > if (TREE_CODE (min) != INTEGER_CST
On 11/10/20 3:35 PM, Richard Sandiford wrote: > Aldy Hernandez <aldyh@redhat.com> writes: >>> (actually I can see 3245 ICEs on aarch64) >>> >>> Can you fix it? >> >> Sure can. >> >> Richard, I seem to have incorrectly removed the early exit for varying, >> and that affected the changes you made for poly ints. Is there any >> reason we can't just exit and set varying without checking for kind != >> VR_VARYING? > > No reason, it seemed more natural to drop to a lower kind with the old > code :-) (But not with the new code.) > > But it isn't obvious to me why the code is now structed the way it is. > > if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max)) > { > set_varying (TREE_TYPE (min)); > return; > } > > // Nothing to canonicalize for symbolic ranges. > if (TREE_CODE (min) != INTEGER_CST > || TREE_CODE (max) != INTEGER_CST) > { > m_kind = kind; > m_base[0] = min; > m_base[1] = max; > m_num_ranges = 1; > return; > } > > swap_out_of_order_endpoints (min, max, kind); > if (kind == VR_VARYING) > { > set_varying (TREE_TYPE (min)); > return; > } > > Why do we want to check “min” and “max” being INTEGER_CST before “kind” > being VR_VARYING, and the potentially record VR_VARYING with specific > bounds? And why do we want to swap the “min” and “max” before checking > whether “kind” is VR_VARYING (when we'll then drop the min and max anyway)? > I think this would benefit from a bit more commentary at least. The main idea was to shorten the code and avoid having to exit due to varying at various points (early and after the operands had been swapped). But yes, it took more cycles. BTW, VR_VARYING does get specific bounds, by design. What could've happened in the code was someone feeding VR_VARYING with non-integer bounds. This would've built an invalid VR_VARYING. How about this (on top of the previous patch which I already pushed to un-break aarch64)? Aldy p.s. If POLY_INT_CST_P are not supported in ranges, but are INTEGRAL_TYPE_P, perhaps we should also tweak irange::supports_type_p so it doesn't leak in anywhere else. gcc/ChangeLog: * value-range.cc (irange::set): Early exit on VR_VARYING. --- gcc/value-range.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gcc/value-range.cc b/gcc/value-range.cc index b7ccba010e4..3703519b03a 100644 --- a/gcc/value-range.cc +++ b/gcc/value-range.cc @@ -249,7 +249,8 @@ irange::set (tree min, tree max, value_range_kind kind) return; } - if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max)) + if (kind == VR_VARYING + || POLY_INT_CST_P (min) || POLY_INT_CST_P (max)) { set_varying (TREE_TYPE (min)); return;
Aldy Hernandez <aldyh@redhat.com> writes: > On 11/10/20 3:35 PM, Richard Sandiford wrote: >> Aldy Hernandez <aldyh@redhat.com> writes: >>>> (actually I can see 3245 ICEs on aarch64) >>>> >>>> Can you fix it? >>> >>> Sure can. >>> >>> Richard, I seem to have incorrectly removed the early exit for varying, >>> and that affected the changes you made for poly ints. Is there any >>> reason we can't just exit and set varying without checking for kind != >>> VR_VARYING? >> >> No reason, it seemed more natural to drop to a lower kind with the old >> code :-) (But not with the new code.) >> >> But it isn't obvious to me why the code is now structed the way it is. >> >> if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max)) >> { >> set_varying (TREE_TYPE (min)); >> return; >> } >> >> // Nothing to canonicalize for symbolic ranges. >> if (TREE_CODE (min) != INTEGER_CST >> || TREE_CODE (max) != INTEGER_CST) >> { >> m_kind = kind; >> m_base[0] = min; >> m_base[1] = max; >> m_num_ranges = 1; >> return; >> } >> >> swap_out_of_order_endpoints (min, max, kind); >> if (kind == VR_VARYING) >> { >> set_varying (TREE_TYPE (min)); >> return; >> } >> >> Why do we want to check “min” and “max” being INTEGER_CST before “kind” >> being VR_VARYING, and the potentially record VR_VARYING with specific >> bounds? And why do we want to swap the “min” and “max” before checking >> whether “kind” is VR_VARYING (when we'll then drop the min and max anyway)? >> I think this would benefit from a bit more commentary at least. > > The main idea was to shorten the code and avoid having to exit due to > varying at various points (early and after the operands had been > swapped). But yes, it took more cycles. > > BTW, VR_VARYING does get specific bounds, by design. What could've > happened in the code was someone feeding VR_VARYING with non-integer > bounds. This would've built an invalid VR_VARYING. > > How about this (on top of the previous patch which I already pushed to > un-break aarch64)? Thanks, this certainly makes the flow clearer for a range noob like me :-) > p.s. If POLY_INT_CST_P are not supported in ranges, but are > INTEGRAL_TYPE_P, perhaps we should also tweak irange::supports_type_p so > it doesn't leak in anywhere else. POLY_INT_CSTs aren't associated with separate types. They're just values of normal integer type. Logically they come somewhere between an INTEGER_CST and an SSA_NAME: they're not “as constant as” an INTEGER_CST but not “as variable as” an SSA_NAME. This means that they really could be treated as symbolic. The difficulty is that POLY_INT_CSTs satisfy is_gimple_min_invariant, and the VRP code fundamentally assumes that is_gimple_min_invariant on an integer type means that the value must be an INTEGER_CST or an ADDR_EXPR. At one point I'd posted a patch to add a new predicate specifically for that, but Richard's response was that noone would remember to use it (which is a fair comment :-)). So the current approach is instead to stop POLY_INT_CSTs getting into the VRP system in the first place. If the VRP code was rigorous about checking for INTEGER_CST before assuming that something was an INTEGER_CST then no special handling of POLY_INT_CST would be needed. But that's not the style that GCC generally follows and trying to retrofit it now seems like fighting against the tide. > diff --git a/gcc/value-range.cc b/gcc/value-range.cc > index b7ccba010e4..3703519b03a 100644 > --- a/gcc/value-range.cc > +++ b/gcc/value-range.cc > @@ -249,7 +249,8 @@ irange::set (tree min, tree max, value_range_kind kind) > return; > } > > - if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max)) > + if (kind == VR_VARYING > + || POLY_INT_CST_P (min) || POLY_INT_CST_P (max)) > { > set_varying (TREE_TYPE (min)); > return; Very minor nit, sorry, but: formatting rules say that all checks should be on one line or that there should be exactly one check per line. OK with that change. Thanks for the quick response in fixing this. Richard
On 11/11/20 10:45 AM, Richard Sandiford wrote: > Aldy Hernandez <aldyh@redhat.com> writes: >> On 11/10/20 3:35 PM, Richard Sandiford wrote: >>> Aldy Hernandez <aldyh@redhat.com> writes: >> p.s. If POLY_INT_CST_P are not supported in ranges, but are >> INTEGRAL_TYPE_P, perhaps we should also tweak irange::supports_type_p so >> it doesn't leak in anywhere else. > > POLY_INT_CSTs aren't associated with separate types. They're just > values of normal integer type. Logically they come somewhere between an > INTEGER_CST and an SSA_NAME: they're not “as constant as” an INTEGER_CST > but not “as variable as” an SSA_NAME. > > This means that they really could be treated as symbolic. The difficulty > is that POLY_INT_CSTs satisfy is_gimple_min_invariant, and the VRP code > fundamentally assumes that is_gimple_min_invariant on an integer type > means that the value must be an INTEGER_CST or an ADDR_EXPR. At one > point I'd posted a patch to add a new predicate specifically for that, > but Richard's response was that noone would remember to use it (which is > a fair comment :-)). So the current approach is instead to stop > POLY_INT_CSTs getting into the VRP system in the first place. > > If the VRP code was rigorous about checking for INTEGER_CST before assuming > that something was an INTEGER_CST then no special handling of POLY_INT_CST > would be needed. But that's not the style that GCC generally follows and > trying to retrofit it now seems like fighting against the tide. I agree, we should probably treat POLY_INTs as symbolics (or varying) right at the setter and avoid dealing with them. Andrew may have a plan for these later, but this will do for now. > >> diff --git a/gcc/value-range.cc b/gcc/value-range.cc >> index b7ccba010e4..3703519b03a 100644 >> --- a/gcc/value-range.cc >> +++ b/gcc/value-range.cc >> @@ -249,7 +249,8 @@ irange::set (tree min, tree max, value_range_kind kind) >> return; >> } >> >> - if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max)) >> + if (kind == VR_VARYING >> + || POLY_INT_CST_P (min) || POLY_INT_CST_P (max)) >> { >> set_varying (TREE_TYPE (min)); >> return; > > Very minor nit, sorry, but: formatting rules say that all checks should > be on one line or that there should be exactly one check per line. Huh... I didn't know we had a rule for that, but it makes perfect sense. I'll keep that in mind for future changes. > > OK with that change. Thanks for the quick response in fixing this. Thanks. Aldy
diff --git a/gcc/value-range.cc b/gcc/value-range.cc index 5827e812216..2124e229e0c 100644 --- a/gcc/value-range.cc +++ b/gcc/value-range.cc @@ -131,13 +131,14 @@ irange::copy_to_legacy (const irange &src) set (src.tree_lower_bound (), src.tree_upper_bound ()); } -// Swap min/max if they are out of order. Return TRUE if further -// processing of the range is necessary, FALSE otherwise. +// Swap MIN/MAX if they are out of order and adjust KIND appropriately. -bool -irange::swap_out_of_order_endpoints (tree &min, tree &max, - value_range_kind &kind) +static void +swap_out_of_order_endpoints (tree &min, tree &max, value_range_kind &kind) { + gcc_checking_assert (kind != VR_UNDEFINED); + if (kind == VR_VARYING) + return; /* Wrong order for min and max, to swap them and the VR type we need to adjust them. */ if (tree_int_cst_lt (max, min)) @@ -149,8 +150,8 @@ irange::swap_out_of_order_endpoints (tree &min, tree &max, for VR_ANTI_RANGE empty range, so drop to varying as well. */ if (TYPE_PRECISION (TREE_TYPE (min)) == 1) { - set_varying (TREE_TYPE (min)); - return false; + kind = VR_VARYING; + return; } one = build_int_cst (TREE_TYPE (min), 1); @@ -163,12 +164,11 @@ irange::swap_out_of_order_endpoints (tree &min, tree &max, to varying in this case. */ if (tree_int_cst_lt (max, min)) { - set_varying (TREE_TYPE (min)); - return false; + kind = VR_VARYING; + return; } kind = kind == VR_RANGE ? VR_ANTI_RANGE : VR_RANGE; } - return true; } void @@ -253,13 +253,6 @@ irange::set (tree min, tree max, value_range_kind kind) && (POLY_INT_CST_P (min) || POLY_INT_CST_P (max))) kind = VR_VARYING; - if (kind == VR_VARYING) - { - set_varying (TREE_TYPE (min)); - return; - } - - tree type = TREE_TYPE (min); // Nothing to canonicalize for symbolic ranges. if (TREE_CODE (min) != INTEGER_CST || TREE_CODE (max) != INTEGER_CST) @@ -270,8 +263,13 @@ irange::set (tree min, tree max, value_range_kind kind) m_num_ranges = 1; return; } - if (!swap_out_of_order_endpoints (min, max, kind)) - goto cleanup_set; + + swap_out_of_order_endpoints (min, max, kind); + if (kind == VR_VARYING) + { + set_varying (TREE_TYPE (min)); + return; + } // Anti-ranges that can be represented as ranges should be so. if (kind == VR_ANTI_RANGE) @@ -280,6 +278,7 @@ irange::set (tree min, tree max, value_range_kind kind) values < -INF and values > INF as -INF/INF as well. */ bool is_min = vrp_val_is_min (min); bool is_max = vrp_val_is_max (max); + tree type = TREE_TYPE (min); if (is_min && is_max) { @@ -314,38 +313,17 @@ irange::set (tree min, tree max, value_range_kind kind) kind = VR_RANGE; } } - else if (!swap_out_of_order_endpoints (min, max, kind)) - goto cleanup_set; - - /* Do not drop [-INF(OVF), +INF(OVF)] to varying. (OVF) has to be sticky - to make sure VRP iteration terminates, otherwise we can get into - oscillations. */ - if (!normalize_min_max (type, min, max, kind)) - { - m_kind = kind; - m_base[0] = min; - m_base[1] = max; - m_num_ranges = 1; - if (flag_checking) - verify_range (); - } - cleanup_set: - // Avoid using TYPE_{MIN,MAX}_VALUE because -fstrict-enums can - // restrict those to a subset of what actually fits in the type. - // Instead use the extremes of the type precision - unsigned prec = TYPE_PRECISION (type); - signop sign = TYPE_SIGN (type); - if (wi::eq_p (wi::to_wide (min), wi::min_value (prec, sign)) - && wi::eq_p (wi::to_wide (max), wi::max_value (prec, sign))) - m_kind = VR_VARYING; - else if (undefined_p ()) - m_kind = VR_UNDEFINED; + m_kind = kind; + m_base[0] = min; + m_base[1] = max; + m_num_ranges = 1; + normalize_min_max (); if (flag_checking) verify_range (); } -/* Check the validity of the range. */ +// Check the validity of the range. void irange::verify_range () diff --git a/gcc/value-range.h b/gcc/value-range.h index 760ee772316..a483fc802dd 100644 --- a/gcc/value-range.h +++ b/gcc/value-range.h @@ -111,8 +111,7 @@ protected: void irange_set (tree, tree); void irange_set_anti_range (tree, tree); - bool swap_out_of_order_endpoints (tree &min, tree &max, value_range_kind &); - bool normalize_min_max (tree type, tree min, tree max, value_range_kind); + void normalize_min_max (); bool legacy_mode_p () const; bool legacy_equal_p (const irange &) const; @@ -566,7 +565,7 @@ irange::set_zero (tree type) irange_set (z, z); } -// Normalize [MIN, MAX] into VARYING and ~[MIN, MAX] into UNDEFINED. +// Normalize a range to VARYING or UNDEFINED if possible. // // Avoid using TYPE_{MIN,MAX}_VALUE because -fstrict-enums can // restrict those to a subset of what actually fits in the type. @@ -575,24 +574,23 @@ irange::set_zero (tree type) // whereas if we used TYPE_*_VAL, said function would just punt upon // seeing a VARYING. -inline bool -irange::normalize_min_max (tree type, tree min, tree max, - value_range_kind kind) +inline void +irange::normalize_min_max () { - unsigned prec = TYPE_PRECISION (type); - signop sign = TYPE_SIGN (type); - if (wi::eq_p (wi::to_wide (min), wi::min_value (prec, sign)) - && wi::eq_p (wi::to_wide (max), wi::max_value (prec, sign))) + gcc_checking_assert (legacy_mode_p ()); + gcc_checking_assert (!undefined_p ()); + unsigned prec = TYPE_PRECISION (type ()); + signop sign = TYPE_SIGN (type ()); + if (wi::eq_p (wi::to_wide (min ()), wi::min_value (prec, sign)) + && wi::eq_p (wi::to_wide (max ()), wi::max_value (prec, sign))) { - if (kind == VR_RANGE) - set_varying (type); - else if (kind == VR_ANTI_RANGE) + if (m_kind == VR_RANGE) + set_varying (type ()); + else if (m_kind == VR_ANTI_RANGE) set_undefined (); else gcc_unreachable (); - return true; } - return false; } // Return the maximum value for TYPE.