Message ID | 20396d4d-cc22-eeab-5cea-685fc077b5f7@redhat.com |
---|---|
State | New |
Headers | show |
Series | disentangle range_fold_*ary_expr into various pieces | expand |
On 10/4/19 7:25 AM, Aldy Hernandez wrote: > I promised I would clean up the VRP/range-ops interface once the patch > went in. I was afraid of shuffling things too much initially, because I > was trying to keep to the original structure of > extract_range_from_*ary_expr. Now that things are in place, it's easier > to abstract things out, and clean them up a bit. > > The gist of this patch is to pull out all the different sections of the > VRP/range-ops glue (range_fold_{binary,unary}) into different functions. > In doing this, the overall structure is easier to see, and I was able > to pin point (and remove) a lot of redundant work. > > Unfortunately, the patch is hard to read because of the reshuffling. It > may be easier to just look at the new patched tree-vrp.c starting at > get_range_op_handler() through range_fold_unary_expr(). > > The general idea is that we now have: > > void > range_fold_binary_expr (value_range_base *vr, > enum tree_code code, > tree expr_type, > const value_range_base *vr0_, > const value_range_base *vr1_) > { > if (!supported_types_p (vr, expr_type) > || !defined_ranges_p (vr, vr0_, vr1_)) > return; > const range_operator *op = get_range_op_handler (vr, code, expr_type); > if (!op) > return; > > value_range_base vr0 = drop_undefines_to_varying (vr0_, expr_type); > value_range_base vr1 = drop_undefines_to_varying (vr1_, expr_type); > if (range_fold_binary_symbolics_p (vr, code, expr_type, &vr0, &vr1)) > return; > > *vr = op->fold_range (expr_type, > vr0.normalize_addresses (), > vr1.normalize_addresses ()); > } > > ...which is WAY easier to read than the spaghetti we had before. Note > that all the symbolic stuff is neatly contained in range_fold_*symbolics_p. > > In doing so, I added the ability to normalize addresses into > normalize_symbolics, which centralizes the place where we transform a > non-numeric range into a strictly numeric range. As a reference, > "symbolics" have traditionally been things like [x + 5, 8], but a > reference is not strictly symbolic, but it's still not numeric [&a, &a], > etc. With the current patch, normalize_symbolics will return a numeric > (or varying or undefined). > > [Yeah, we should call that ::normalize_non_numeric or something, perhaps > later.] > > I also exposed the new ::normalize_addresses method, so the > range_fold_*ary* stuff can normalize addresses instead of going through > the entire normalize_symbolics dance. Though, I could just as easily > just call normalize_symbolics from the range_fold* stuff. It just > seemed like a useful small optimization. > > I was pleasantly surprised that the symbolic handling of > CONVERT_EXPR_CODE_P melted in favor of the generic ::normalize_symbolics > mechanism. > > Anyways, sorry for the gymnastics here, but regardless of what happens > to range_fold_*ary*, it needed a good clean-up. > > OK? > > Aldy > > range-fold-cleanups.patch > > commit 70d336374fb39e857ecbd720900df9869c3c2ce9 > Author: Aldy Hernandez <aldyh@redhat.com> > Date: Thu Oct 3 14:53:12 2019 +0200 > > Disentangle range_fold_*ary_expr() into various independent pieces. > > diff --git a/gcc/ChangeLog b/gcc/ChangeLog > index 3934b41fdf9..714b22781ee 100644 > --- a/gcc/ChangeLog > +++ b/gcc/ChangeLog > @@ -1,3 +1,17 @@ > +2019-10-04 Aldy Hernandez <aldyh@redhat.com> > + > + * tree-vrp.c (get_range_op_handler): New. > + (supported_types_p): New. > + (defined_ranges_p): New. > + (drop_undefines_to_varying): New. > + (range_fold_binary_symbolics_p): New. > + (range_fold_unary_symbolics_p): New. > + (range_fold_unary_expr): Extract out into above functions. > + (range_fold_binary_expr): Same. > + (value_range_base::normalize_addresses): New. > + (value_range_base::normalize_symbolics): Normalize addresses. > + * tree-vrp.h (class value_range_base): Add normalize_addresses. You're right. Easier to refer to the before/after directly. Sometimes diffs just suck. OK jeff
On 10/4/19 2:09 PM, Jeff Law wrote: > You're right. Easier to refer to the before/after directly. Sometimes > diffs just suck. > > OK > jeff In testing this patch in isolation from the non-zero canonicalization patch, I found one regression due to the fact that: a) As discussed, two non-zero representations currently exist for unsigned ranges. b) ipa-prop.c has it's own hacked up value_range structure (ipa_vr) which doesn't use any API. Since there is no agreed upon non-zero, range-ops can sometimes (correctly) create an unsigned [1,MAX], and ipa-prop.c is open-coding the check for a pointer non-zero to ~[0,0]. This seems like a latent bug. I really have no idea, nor do I care (*), what we do with ipa-prop's lack of API. For now, I have implemented ipa_vr::nonzero_p(), and used it. When we agree on the non-zero normalization we can adjust this method if necessary. +bool +ipa_vr::nonzero_p (tree expr_type) const +{ + if (type == VR_ANTI_RANGE && wi::eq_p (min, 0) && wi::eq_p (max, 0)) + return true; + + unsigned prec = TYPE_PRECISION (expr_type); + return (type == VR_RANGE + && wi::eq_p (min, wi::one (prec)) + && wi::eq_p (max, wi::max_value (prec, TYPE_SIGN (expr_type)))); +} ... else if (POINTER_TYPE_P (TREE_TYPE (ddef)) - && vr[i].type == VR_ANTI_RANGE - && wi::eq_p (vr[i].min, 0) - && wi::eq_p (vr[i].max, 0)) + && vr[i].nonzero_p (TREE_TYPE (ddef))) Attached is the final adjusted patch I have committed to trunk. Aldy (*) This can be loosely translated as "it'll eventually bite us in the ass, at which point I'll end up addressing it." ;-)
> +bool > +ipa_vr::nonzero_p (tree expr_type) const > +{ > + if (type == VR_ANTI_RANGE && wi::eq_p (min, 0) && wi::eq_p (max, 0)) > + return true; > + > + unsigned prec = TYPE_PRECISION (expr_type); > + return (type == VR_RANGE > + && wi::eq_p (min, wi::one (prec)) > + && wi::eq_p (max, wi::max_value (prec, TYPE_SIGN (expr_type)))); > +} Errr, wrong version posted. There was a TYPE_UNSIGNED missing. Fixed and committed. diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index 20a0bddcbab..5020f4a44d5 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -5117,6 +5117,7 @@ ipa_vr::nonzero_p (tree expr_type) const unsigned prec = TYPE_PRECISION (expr_type); return (type == VR_RANGE + && TYPE_UNSIGNED (expr_type) && wi::eq_p (min, wi::one (prec)) && wi::eq_p (max, wi::max_value (prec, TYPE_SIGN (expr_type)))); }
On Mon, 7 Oct 2019, Aldy Hernandez wrote: > On 10/4/19 2:09 PM, Jeff Law wrote: > >> You're right. Easier to refer to the before/after directly. Sometimes >> diffs just suck. >> >> OK >> jeff > > In testing this patch in isolation from the non-zero canonicalization patch, > I found one regression due to the fact that: > > a) As discussed, two non-zero representations currently exist for unsigned > ranges. > > b) ipa-prop.c has it's own hacked up value_range structure (ipa_vr) which > doesn't use any API. Since there is no agreed upon non-zero, range-ops can > sometimes (correctly) create an unsigned [1,MAX], and ipa-prop.c is > open-coding the check for a pointer non-zero to ~[0,0]. This seems like a > latent bug. > > I really have no idea, nor do I care (*), what we do with ipa-prop's lack of > API. For now, I have implemented ipa_vr::nonzero_p(), and used it. When we > agree on the non-zero normalization we can adjust this method if necessary. > > +bool > +ipa_vr::nonzero_p (tree expr_type) const > +{ > + if (type == VR_ANTI_RANGE && wi::eq_p (min, 0) && wi::eq_p (max, 0)) > + return true; > + > + unsigned prec = TYPE_PRECISION (expr_type); > + return (type == VR_RANGE > + && wi::eq_p (min, wi::one (prec)) > + && wi::eq_p (max, wi::max_value (prec, TYPE_SIGN (expr_type)))); > +} > > ... > > else if (POINTER_TYPE_P (TREE_TYPE (ddef)) > - && vr[i].type == VR_ANTI_RANGE > - && wi::eq_p (vr[i].min, 0) > - && wi::eq_p (vr[i].max, 0)) > + && vr[i].nonzero_p (TREE_TYPE (ddef))) > > Attached is the final adjusted patch I have committed to trunk. I wonder why we would ever want to ask "is this interval the one that misses exactly the value 0" instead of "does this interval contain the value 0". I naively believe there shouldn't even be any API for the first question. Or if pointers really only have 2 possible intervals (besides varying and undefined), aka [0,0] and ~[0,0], using intervals seems like overkill for them...
Hi, On Tue, Oct 08 2019, Marc Glisse wrote: > On Mon, 7 Oct 2019, Aldy Hernandez wrote: >> >> In testing this patch in isolation from the non-zero canonicalization patch, >> I found one regression due to the fact that: >> >> a) As discussed, two non-zero representations currently exist for unsigned >> ranges. >> >> b) ipa-prop.c has it's own hacked up value_range structure (ipa_vr) which >> doesn't use any API. Since there is no agreed upon non-zero, range-ops can >> sometimes (correctly) create an unsigned [1,MAX], and ipa-prop.c is >> open-coding the check for a pointer non-zero to ~[0,0]. This seems like a >> latent bug. >> >> I really have no idea, nor do I care (*), what we do with ipa-prop's lack of >> API. For now, I have implemented ipa_vr::nonzero_p(), and used it. When we >> agree on the non-zero normalization we can adjust this method if necessary. >> >> +bool >> +ipa_vr::nonzero_p (tree expr_type) const >> +{ >> + if (type == VR_ANTI_RANGE && wi::eq_p (min, 0) && wi::eq_p (max, 0)) >> + return true; >> + >> + unsigned prec = TYPE_PRECISION (expr_type); >> + return (type == VR_RANGE >> + && wi::eq_p (min, wi::one (prec)) >> + && wi::eq_p (max, wi::max_value (prec, TYPE_SIGN (expr_type)))); >> +} >> >> ... >> >> else if (POINTER_TYPE_P (TREE_TYPE (ddef)) >> - && vr[i].type == VR_ANTI_RANGE >> - && wi::eq_p (vr[i].min, 0) >> - && wi::eq_p (vr[i].max, 0)) >> + && vr[i].nonzero_p (TREE_TYPE (ddef))) >> >> Attached is the final adjusted patch I have committed to trunk. > > I wonder why we would ever want to ask "is this interval the one that > misses exactly the value 0" instead of "does this interval contain the > value 0". I naively believe there shouldn't even be any API for the first > question. Or if pointers really only have 2 possible intervals (besides > varying and undefined), aka [0,0] and ~[0,0], using intervals seems like > overkill for them... > The only use of this code is to see if we can do set_ptr_nonnull (ddef) where ddef is the default definition of a pointer argument described by the value range. For integer arguments, we use the values in ipa_vr to set_range_info of the default definition, so even if it is an overkill for pointers, the data structure cannot be replaced with just a flag. While I know that ~[0,0] is by far the most common pointer ipa_vr, I will have a look whether it makes sense to rewrite the test as you suggested after I pull the changes from trunk. Thanks for pointing it out, Martin
On Mon, 7 Oct 2019 at 13:53, Aldy Hernandez <aldyh@redhat.com> wrote: > > > +bool > > +ipa_vr::nonzero_p (tree expr_type) const > > +{ > > + if (type == VR_ANTI_RANGE && wi::eq_p (min, 0) && wi::eq_p (max, 0)) > > + return true; > > + > > + unsigned prec = TYPE_PRECISION (expr_type); > > + return (type == VR_RANGE > > + && wi::eq_p (min, wi::one (prec)) > > + && wi::eq_p (max, wi::max_value (prec, TYPE_SIGN > (expr_type)))); > > +} > > Errr, wrong version posted. There was a TYPE_UNSIGNED missing. > > Fixed and committed. > > Hi, Since this was committed (r276654), I've noticed regressions on aarch64: gcc.target/aarch64/pr88838.c (test for excess errors) gcc.target/aarch64/stack-check-cfa-3.c (test for excess errors) gcc.target/aarch64/stack-check-prologue-16.c (test for excess errors) gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve scan-assembler-times \\tabs\\tz[0-9]+\\.b, p[0-7]/m, z[0-9]+\\.b\\n 1 gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve scan-assembler-times \\tabs\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d\\n 1 gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve scan-assembler-times \\tabs\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h\\n 1 gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve scan-assembler-times \\tabs\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s\\n 1 gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve (test for excess errors) gcc.target/aarch64/sve/adr_1.c -march=armv8.2-a+sve (test for excess errors) and many others For instance: compiler exited with status 1 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:5795 0x5efa71 tree_check_failed(tree_node const*, char const*, int, char const*, ...) /gcc/tree.c:9926 0x749584 tree_check(tree_node const*, char const*, int, char const*, tree_code) /gcc/tree.h:3523 0x749584 wi::to_wide(tree_node const*) /gcc/tree.h:5795 0xf7c0c0 value_range_base::lower_bound(unsigned int) const /gcc/tree-vrp.c:6136 0x155d2e6 range_operator::fold_range(tree_node*, value_range_base const&, value_range_base const&) const /gcc/range-op.cc:156 0xf87597 range_fold_binary_expr(value_range_base*, tree_code, tree_node*, value_range_base const*, value_range_base const*) /gcc/tree-vrp.c:1915 0x1007962 vr_values::extract_range_from_binary_expr(value_range*, tree_code, tree_node*, tree_node*, tree_node*) /gcc/vr-values.c:808 0x1011f0c vr_values::extract_range_from_assignment(value_range*, gassign*) /gcc/vr-values.c:1469 0x1499d21 evrp_range_analyzer::record_ranges_from_stmt(gimple*, bool) /gcc/gimple-ssa-evrp-analyze.c:307 0xdd76dd dom_opt_dom_walker::before_dom_children(basic_block_def*) /gcc/tree-ssa-dom.c:1503 0x146e68a dom_walker::walk(basic_block_def*) /gcc/domwalk.c:309 0xdd4759 execute /gcc/tree-ssa-dom.c:724 gcc.target/aarch64/pr88838.c: output file does not exist UNRESOLVED: gcc.target/aarch64/pr88838.c scan-assembler-not sxtw Christophe diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c > index 20a0bddcbab..5020f4a44d5 100644 > --- a/gcc/ipa-prop.c > +++ b/gcc/ipa-prop.c > @@ -5117,6 +5117,7 @@ ipa_vr::nonzero_p (tree expr_type) const > > unsigned prec = TYPE_PRECISION (expr_type); > return (type == VR_RANGE > + && TYPE_UNSIGNED (expr_type) > && wi::eq_p (min, wi::one (prec)) > && wi::eq_p (max, wi::max_value (prec, TYPE_SIGN (expr_type)))); > } >
It's not that particular commit, but the previous one, but yeah, that's all me. (gdb) #4 0x000000000145bf44 in value_range_base::lower_bound (this=0x7fffffffc360, pair=0) at /home/cygnus/aldyh/src/gcc/gcc/tree-vrp.c:6148 6148 return wi::to_wide (t); (gdb) #5 0x0000000001ccb1d9 in range_operator::fold_range ( this=0x271edc8 <op_plus>, type=<integer_type 0x7fffefd451f8>, lh=..., rh=...) at /home/cygnus/aldyh/src/gcc/gcc/range-op.cc:156 156 wide_int rh_lb = rh.lower_bound (y); (gdb) #6 0x000000000144b850 in range_fold_binary_expr (vr=0x7fffffffc6c0, code=PLUS_EXPR, expr_type=<integer_type 0x7fffefd451f8>, vr0_=0x7fffffffc480, vr1_=0x7fffffffc460) at /home/cygnus/aldyh/src/gcc/gcc/tree-vrp.c:1927 1927 vr1.normalize_addresses ()); (gdb) l 1922 if (range_fold_binary_symbolics_p (vr, code, expr_type, &vr0, &vr1)) 1923 return; 1924 1925 *vr = op->fold_range (expr_type, 1926 vr0.normalize_addresses (), 1927 vr1.normalize_addresses ()); 1928 } 1929 1930 /* Perform a unary operation on a range. */ 1931 (gdb) print vr1.dump() unsigned long [POLY_INT_CST [4, 4], POLY_INT_CST [4, 4]]$1 = void (gdb) We have a value_range_base containing a POLYT_INT_CST which we don't handle. That's rather unfortunate. I was hoping by this point, anything we couldn't handle was just an address. The previous range-ops code was normalizing anything not INTEGER_CST into VARYING, which I thought was a rather big hammer, and was trying to avoid. Interesting that the old VRP code also allowing the POLY_INT to pass on through all the way into extract_range_binary_expr, only to bail when it see's it's not an INTEGER_CST: && (TREE_CODE (max_op0) == INTEGER_CST || (sym_max_op0 = get_single_symbol (max_op0, &neg_max_op0, &max_op0))) && (TREE_CODE (max_op1) == INTEGER_CST || (sym_max_op1 = get_single_symbol (max_op1, &neg_max_op1, &max_op1))) I guess we could bring out the big hammer again if normalize_addresses doesn't give us an INTEGER_CST. I'll take a look at this. Thanks for pointing it out. Aldy On 10/10/19 5:25 AM, Christophe Lyon wrote: > > > On Mon, 7 Oct 2019 at 13:53, Aldy Hernandez <aldyh@redhat.com > <mailto:aldyh@redhat.com>> wrote: > > > > +bool > > +ipa_vr::nonzero_p (tree expr_type) const > > +{ > > + if (type == VR_ANTI_RANGE && wi::eq_p (min, 0) && wi::eq_p > (max, 0)) > > + return true; > > + > > + unsigned prec = TYPE_PRECISION (expr_type); > > + return (type == VR_RANGE > > + && wi::eq_p (min, wi::one (prec)) > > + && wi::eq_p (max, wi::max_value (prec, TYPE_SIGN > (expr_type)))); > > +} > > Errr, wrong version posted. There was a TYPE_UNSIGNED missing. > > Fixed and committed. > > > Hi, > Since this was committed (r276654), I've noticed regressions on aarch64: > gcc.target/aarch64/pr88838.c (test for excess errors) > gcc.target/aarch64/stack-check-cfa-3.c (test for excess errors) > gcc.target/aarch64/stack-check-prologue-16.c (test for excess errors) > gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve > scan-assembler-times \\tabs\\tz[0-9]+\\.b, p[0-7]/m, z[0-9]+\\.b\\n 1 > gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve > scan-assembler-times \\tabs\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d\\n 1 > gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve > scan-assembler-times \\tabs\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h\\n 1 > gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve > scan-assembler-times \\tabs\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s\\n 1 > gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve (test for > excess errors) > gcc.target/aarch64/sve/adr_1.c -march=armv8.2-a+sve (test for > excess errors) > > and many others > > For instance: > compiler exited with status 1 > 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:5795 > 0x5efa71 tree_check_failed(tree_node const*, char const*, int, char > const*, ...) > /gcc/tree.c:9926 > 0x749584 tree_check(tree_node const*, char const*, int, char const*, > tree_code) > /gcc/tree.h:3523 > 0x749584 wi::to_wide(tree_node const*) > /gcc/tree.h:5795 > 0xf7c0c0 value_range_base::lower_bound(unsigned int) const > /gcc/tree-vrp.c:6136 > 0x155d2e6 range_operator::fold_range(tree_node*, value_range_base > const&, value_range_base const&) const > /gcc/range-op.cc:156 > 0xf87597 range_fold_binary_expr(value_range_base*, tree_code, > tree_node*, value_range_base const*, value_range_base const*) > /gcc/tree-vrp.c:1915 > 0x1007962 vr_values::extract_range_from_binary_expr(value_range*, > tree_code, tree_node*, tree_node*, tree_node*) > /gcc/vr-values.c:808 > 0x1011f0c vr_values::extract_range_from_assignment(value_range*, gassign*) > /gcc/vr-values.c:1469 > 0x1499d21 evrp_range_analyzer::record_ranges_from_stmt(gimple*, bool) > /gcc/gimple-ssa-evrp-analyze.c:307 > 0xdd76dd dom_opt_dom_walker::before_dom_children(basic_block_def*) > /gcc/tree-ssa-dom.c:1503 > 0x146e68a dom_walker::walk(basic_block_def*) > /gcc/domwalk.c:309 > 0xdd4759 execute > /gcc/tree-ssa-dom.c:724 > > gcc.target/aarch64/pr88838.c: output file does not exist > UNRESOLVED: gcc.target/aarch64/pr88838.c scan-assembler-not sxtw > > Christophe > > diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c > index 20a0bddcbab..5020f4a44d5 100644 > --- a/gcc/ipa-prop.c > +++ b/gcc/ipa-prop.c > @@ -5117,6 +5117,7 @@ ipa_vr::nonzero_p (tree expr_type) const > > unsigned prec = TYPE_PRECISION (expr_type); > return (type == VR_RANGE > + && TYPE_UNSIGNED (expr_type) > && wi::eq_p (min, wi::one (prec)) > && wi::eq_p (max, wi::max_value (prec, TYPE_SIGN > (expr_type)))); > } >
On Thu, 10 Oct 2019 at 17:19, Aldy Hernandez <aldyh@redhat.com> wrote: > It's not that particular commit, but the previous one, but yeah, that's > all me. Interesting, bisect really identified r276654. I think that's what Steve reported as PR 92051 (although his backtrace is different, but it's also for a different testcase). > (gdb) > #4 0x000000000145bf44 in value_range_base::lower_bound > (this=0x7fffffffc360, > pair=0) at /home/cygnus/aldyh/src/gcc/gcc/tree-vrp.c:6148 > 6148 return wi::to_wide (t); > (gdb) > #5 0x0000000001ccb1d9 in range_operator::fold_range ( > this=0x271edc8 <op_plus>, type=<integer_type 0x7fffefd451f8>, lh=..., > rh=...) at /home/cygnus/aldyh/src/gcc/gcc/range-op.cc:156 > 156 wide_int rh_lb = rh.lower_bound (y); > (gdb) > #6 0x000000000144b850 in range_fold_binary_expr (vr=0x7fffffffc6c0, > code=PLUS_EXPR, expr_type=<integer_type 0x7fffefd451f8>, > vr0_=0x7fffffffc480, vr1_=0x7fffffffc460) > at /home/cygnus/aldyh/src/gcc/gcc/tree-vrp.c:1927 > 1927 vr1.normalize_addresses ()); > (gdb) l > 1922 if (range_fold_binary_symbolics_p (vr, code, expr_type, &vr0, > &vr1)) > 1923 return; > 1924 > 1925 *vr = op->fold_range (expr_type, > 1926 vr0.normalize_addresses (), > 1927 vr1.normalize_addresses ()); > 1928 } > 1929 > 1930 /* Perform a unary operation on a range. */ > 1931 > (gdb) print vr1.dump() > unsigned long [POLY_INT_CST [4, 4], POLY_INT_CST [4, 4]]$1 = void > (gdb) > > We have a value_range_base containing a POLYT_INT_CST which we don't > handle. > > That's rather unfortunate. I was hoping by this point, anything we > couldn't handle was just an address. The previous range-ops code was > normalizing anything not INTEGER_CST into VARYING, which I thought was a > rather big hammer, and was trying to avoid. > > Interesting that the old VRP code also allowing the POLY_INT to pass on > through all the way into extract_range_binary_expr, only to bail when it > see's it's not an INTEGER_CST: > > && (TREE_CODE (max_op0) == INTEGER_CST > || (sym_max_op0 > = get_single_symbol (max_op0, &neg_max_op0, &max_op0))) > && (TREE_CODE (max_op1) == INTEGER_CST > || (sym_max_op1 > = get_single_symbol (max_op1, &neg_max_op1, &max_op1))) > > I guess we could bring out the big hammer again if normalize_addresses > doesn't give us an INTEGER_CST. > > I'll take a look at this. Thanks for pointing it out. > > Thanks, Christophe > Aldy > > On 10/10/19 5:25 AM, Christophe Lyon wrote: > > > > > > On Mon, 7 Oct 2019 at 13:53, Aldy Hernandez <aldyh@redhat.com > > <mailto:aldyh@redhat.com>> wrote: > > > > > > > +bool > > > +ipa_vr::nonzero_p (tree expr_type) const > > > +{ > > > + if (type == VR_ANTI_RANGE && wi::eq_p (min, 0) && wi::eq_p > > (max, 0)) > > > + return true; > > > + > > > + unsigned prec = TYPE_PRECISION (expr_type); > > > + return (type == VR_RANGE > > > + && wi::eq_p (min, wi::one (prec)) > > > + && wi::eq_p (max, wi::max_value (prec, TYPE_SIGN > > (expr_type)))); > > > +} > > > > Errr, wrong version posted. There was a TYPE_UNSIGNED missing. > > > > Fixed and committed. > > > > > > Hi, > > Since this was committed (r276654), I've noticed regressions on aarch64: > > gcc.target/aarch64/pr88838.c (test for excess errors) > > gcc.target/aarch64/stack-check-cfa-3.c (test for excess errors) > > gcc.target/aarch64/stack-check-prologue-16.c (test for excess > errors) > > gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve > > scan-assembler-times \\tabs\\tz[0-9]+\\.b, p[0-7]/m, z[0-9]+\\.b\\n 1 > > gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve > > scan-assembler-times \\tabs\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d\\n 1 > > gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve > > scan-assembler-times \\tabs\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h\\n 1 > > gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve > > scan-assembler-times \\tabs\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s\\n 1 > > gcc.target/aarch64/sve/abs_1.c -march=armv8.2-a+sve (test for > > excess errors) > > gcc.target/aarch64/sve/adr_1.c -march=armv8.2-a+sve (test for > > excess errors) > > > > and many others > > > > For instance: > > compiler exited with status 1 > > 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:5795 > > 0x5efa71 tree_check_failed(tree_node const*, char const*, int, char > > const*, ...) > > /gcc/tree.c:9926 > > 0x749584 tree_check(tree_node const*, char const*, int, char const*, > > tree_code) > > /gcc/tree.h:3523 > > 0x749584 wi::to_wide(tree_node const*) > > /gcc/tree.h:5795 > > 0xf7c0c0 value_range_base::lower_bound(unsigned int) const > > /gcc/tree-vrp.c:6136 > > 0x155d2e6 range_operator::fold_range(tree_node*, value_range_base > > const&, value_range_base const&) const > > /gcc/range-op.cc:156 > > 0xf87597 range_fold_binary_expr(value_range_base*, tree_code, > > tree_node*, value_range_base const*, value_range_base const*) > > /gcc/tree-vrp.c:1915 > > 0x1007962 vr_values::extract_range_from_binary_expr(value_range*, > > tree_code, tree_node*, tree_node*, tree_node*) > > /gcc/vr-values.c:808 > > 0x1011f0c vr_values::extract_range_from_assignment(value_range*, > gassign*) > > /gcc/vr-values.c:1469 > > 0x1499d21 evrp_range_analyzer::record_ranges_from_stmt(gimple*, bool) > > /gcc/gimple-ssa-evrp-analyze.c:307 > > 0xdd76dd dom_opt_dom_walker::before_dom_children(basic_block_def*) > > /gcc/tree-ssa-dom.c:1503 > > 0x146e68a dom_walker::walk(basic_block_def*) > > /gcc/domwalk.c:309 > > 0xdd4759 execute > > /gcc/tree-ssa-dom.c:724 > > > > gcc.target/aarch64/pr88838.c: output file does not exist > > UNRESOLVED: gcc.target/aarch64/pr88838.c scan-assembler-not sxtw > > > > Christophe > > > > diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c > > index 20a0bddcbab..5020f4a44d5 100644 > > --- a/gcc/ipa-prop.c > > +++ b/gcc/ipa-prop.c > > @@ -5117,6 +5117,7 @@ ipa_vr::nonzero_p (tree expr_type) const > > > > unsigned prec = TYPE_PRECISION (expr_type); > > return (type == VR_RANGE > > + && TYPE_UNSIGNED (expr_type) > > && wi::eq_p (min, wi::one (prec)) > > && wi::eq_p (max, wi::max_value (prec, TYPE_SIGN > > (expr_type)))); > > } > > >
commit 70d336374fb39e857ecbd720900df9869c3c2ce9 Author: Aldy Hernandez <aldyh@redhat.com> Date: Thu Oct 3 14:53:12 2019 +0200 Disentangle range_fold_*ary_expr() into various independent pieces. diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 3934b41fdf9..714b22781ee 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,17 @@ +2019-10-04 Aldy Hernandez <aldyh@redhat.com> + + * tree-vrp.c (get_range_op_handler): New. + (supported_types_p): New. + (defined_ranges_p): New. + (drop_undefines_to_varying): New. + (range_fold_binary_symbolics_p): New. + (range_fold_unary_symbolics_p): New. + (range_fold_unary_expr): Extract out into above functions. + (range_fold_binary_expr): Same. + (value_range_base::normalize_addresses): New. + (value_range_base::normalize_symbolics): Normalize addresses. + * tree-vrp.h (class value_range_base): Add normalize_addresses. + 2019-10-04 Aldy Hernandez <aldyh@redhat.com> * tree-vrp.c (value_range_base::singleton_p): Use num_pairs diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c index 97dd2b7a734..4370c46e966 100644 --- a/gcc/tree-vrp.c +++ b/gcc/tree-vrp.c @@ -1778,119 +1778,102 @@ extract_range_from_plus_minus_expr (value_range_base *vr, vr->set (kind, min, max); } -/* Normalize a value_range for use in range_ops and return it. */ +/* Return the range-ops handler for CODE and EXPR_TYPE. If no + suitable operator is found, return NULL and set VR to VARYING. */ -static value_range_base -normalize_for_range_ops (const value_range_base &vr) +static const range_operator * +get_range_op_handler (value_range_base *vr, + enum tree_code code, + tree expr_type) { - tree type = vr.type (); + const range_operator *op = range_op_handler (code, expr_type); + if (!op) + vr->set_varying (expr_type); + return op; +} + +/* If the types passed are supported, return TRUE, otherwise set VR to + VARYING and return FALSE. */ - /* This will return ~[0,0] for [&var, &var]. */ - if (POINTER_TYPE_P (type) && !range_includes_zero_p (&vr)) +static bool +supported_types_p (value_range_base *vr, + tree type0, + tree type1 = NULL) +{ + if (!value_range_base::supports_type_p (type0) + || (type1 && !value_range_base::supports_type_p (type1))) { - value_range_base temp; - temp.set_nonzero (type); - return temp; + vr->set_varying (type0); + return false; } - if (vr.symbolic_p ()) - return normalize_for_range_ops (vr.normalize_symbolics ()); - if (TREE_CODE (vr.min ()) == INTEGER_CST - && TREE_CODE (vr.max ()) == INTEGER_CST) - return vr; - /* Anything not strictly numeric at this point becomes varying. */ - return value_range_base (vr.type ()); + return true; } -/* Fold a binary expression of two value_range's with range-ops. */ +/* If any of the ranges passed are defined, return TRUE, otherwise set + VR to UNDEFINED and return FALSE. */ -void -range_fold_binary_expr (value_range_base *vr, - enum tree_code code, - tree expr_type, - const value_range_base *vr0_, - const value_range_base *vr1_) +static bool +defined_ranges_p (value_range_base *vr, + const value_range_base *vr0, + const value_range_base *vr1 = NULL) { - if (!value_range_base::supports_type_p (expr_type) - || (!vr0_->undefined_p () - && !value_range_base::supports_type_p (vr0_->type ())) - || (!vr1_->undefined_p () - && !value_range_base::supports_type_p (vr1_->type ()))) - { - vr->set_varying (expr_type); - return; - } - if (vr0_->undefined_p () && vr1_->undefined_p ()) + if (vr0->undefined_p () && (!vr1 || vr1->undefined_p ())) { vr->set_undefined (); - return; - } - range_operator *op = range_op_handler (code, expr_type); - if (!op) - { - vr->set_varying (expr_type); - return; + return false; } + return true; +} - /* Mimic any behavior users of extract_range_from_binary_expr may - expect. */ - value_range_base vr0 = *vr0_, vr1 = *vr1_; - if (vr0.undefined_p ()) - vr0.set_varying (expr_type); - else if (vr1.undefined_p ()) - vr1.set_varying (expr_type); +static value_range_base +drop_undefines_to_varying (const value_range_base *vr, tree expr_type) +{ + if (vr->undefined_p ()) + return value_range_base (expr_type); + else + return *vr; +} + +/* If any operand is symbolic, perform a binary operation on them and + return TRUE, otherwise return FALSE. */ - /* Handle symbolics. */ - if (vr0.symbolic_p () || vr1.symbolic_p ()) +static bool +range_fold_binary_symbolics_p (value_range_base *vr, + tree_code code, + tree expr_type, + const value_range_base *vr0, + const value_range_base *vr1) +{ + if (vr0->symbolic_p () || vr1->symbolic_p ()) { if ((code == PLUS_EXPR || code == MINUS_EXPR)) { - extract_range_from_plus_minus_expr (vr, code, expr_type, - &vr0, &vr1); - return; + extract_range_from_plus_minus_expr (vr, code, expr_type, vr0, vr1); + return true; } if (POINTER_TYPE_P (expr_type) && code == POINTER_PLUS_EXPR) { - extract_range_from_pointer_plus_expr (vr, code, expr_type, - &vr0, &vr1); - return; + extract_range_from_pointer_plus_expr (vr, code, expr_type, vr0, vr1); + return true; } + const range_operator *op = get_range_op_handler (vr, code, expr_type); + *vr = op->fold_range (expr_type, + vr0->normalize_symbolics (), + vr1->normalize_symbolics ()); + return true; } - - /* Do the range-ops dance. */ - value_range_base n0 = normalize_for_range_ops (vr0); - value_range_base n1 = normalize_for_range_ops (vr1); - *vr = op->fold_range (expr_type, n0, n1); + return false; } -/* Fold a unary expression of a value_range with range-ops. */ +/* If operand is symbolic, perform a unary operation on it and return + TRUE, otherwise return FALSE. */ -void -range_fold_unary_expr (value_range_base *vr, - enum tree_code code, tree expr_type, - const value_range_base *vr0, - tree vr0_type) +static bool +range_fold_unary_symbolics_p (value_range_base *vr, + tree_code code, + tree expr_type, + const value_range_base *vr0) { - /* Mimic any behavior users of extract_range_from_unary_expr may - expect. */ - if (!value_range_base::supports_type_p (expr_type) - || !value_range_base::supports_type_p (vr0_type)) - { - vr->set_varying (expr_type); - return; - } - if (vr0->undefined_p ()) - { - vr->set_undefined (); - return; - } - range_operator *op = range_op_handler (code, expr_type); - if (!op) - { - vr->set_varying (expr_type); - return; - } - - /* Handle symbolics. */ if (vr0->symbolic_p ()) { if (code == NEGATE_EXPR) @@ -1899,7 +1882,7 @@ range_fold_unary_expr (value_range_base *vr, value_range_base zero; zero.set_zero (vr0->type ()); range_fold_binary_expr (vr, MINUS_EXPR, expr_type, &zero, vr0); - return; + return true; } if (code == BIT_NOT_EXPR) { @@ -1907,30 +1890,64 @@ range_fold_unary_expr (value_range_base *vr, value_range_base minusone; minusone.set (build_int_cst (vr0->type (), -1)); range_fold_binary_expr (vr, MINUS_EXPR, expr_type, &minusone, vr0); - return; + return true; } + const range_operator *op = get_range_op_handler (vr, code, expr_type); *vr = op->fold_range (expr_type, - normalize_for_range_ops (*vr0), + vr0->normalize_symbolics (), value_range_base (expr_type)); - return; - } - if (CONVERT_EXPR_CODE_P (code) && (POINTER_TYPE_P (expr_type) - || POINTER_TYPE_P (vr0->type ()))) - { - /* This handles symbolic conversions such such as [25, x_4]. */ - if (!range_includes_zero_p (vr0)) - vr->set_nonzero (expr_type); - else if (vr0->zero_p ()) - vr->set_zero (expr_type); - else - vr->set_varying (expr_type); - return; + return true; } + return false; +} + +/* Perform a binary operation on a pair of ranges. */ - /* Do the range-ops dance. */ - value_range_base n0 = normalize_for_range_ops (*vr0); - value_range_base n1 (expr_type); - *vr = op->fold_range (expr_type, n0, n1); +void +range_fold_binary_expr (value_range_base *vr, + enum tree_code code, + tree expr_type, + const value_range_base *vr0_, + const value_range_base *vr1_) +{ + if (!supported_types_p (vr, expr_type) + || !defined_ranges_p (vr, vr0_, vr1_)) + return; + const range_operator *op = get_range_op_handler (vr, code, expr_type); + if (!op) + return; + + value_range_base vr0 = drop_undefines_to_varying (vr0_, expr_type); + value_range_base vr1 = drop_undefines_to_varying (vr1_, expr_type); + if (range_fold_binary_symbolics_p (vr, code, expr_type, &vr0, &vr1)) + return; + + *vr = op->fold_range (expr_type, + vr0.normalize_addresses (), + vr1.normalize_addresses ()); +} + +/* Perform a unary operation on a range. */ + +void +range_fold_unary_expr (value_range_base *vr, + enum tree_code code, tree expr_type, + const value_range_base *vr0, + tree vr0_type) +{ + if (!supported_types_p (vr, expr_type, vr0_type) + || !defined_ranges_p (vr, vr0)) + return; + const range_operator *op = get_range_op_handler (vr, code, expr_type); + if (!op) + return; + + if (range_fold_unary_symbolics_p (vr, code, expr_type, vr0)) + return; + + *vr = op->fold_range (expr_type, + vr0->normalize_addresses (), + value_range_base (expr_type)); } /* Given a COND_EXPR COND of the form 'V OP W', and an SSA name V, @@ -6015,7 +6032,24 @@ value_range::union_ (const value_range *other) } } -/* Normalize symbolics into constants. */ +/* Normalize addresses into constants. */ + +value_range_base +value_range_base::normalize_addresses () const +{ + if (!POINTER_TYPE_P (type ()) || range_has_numeric_bounds_p (this)) + return *this; + + if (!range_includes_zero_p (this)) + { + gcc_checking_assert (TREE_CODE (m_min) == ADDR_EXPR + || TREE_CODE (m_max) == ADDR_EXPR); + return range_nonzero (type ()); + } + return value_range_base (type ()); +} + +/* Normalize symbolics and addresses into constants. */ value_range_base value_range_base::normalize_symbolics () const @@ -6026,7 +6060,7 @@ value_range_base::normalize_symbolics () const bool min_symbolic = !is_gimple_min_invariant (min ()); bool max_symbolic = !is_gimple_min_invariant (max ()); if (!min_symbolic && !max_symbolic) - return *this; + return normalize_addresses (); // [SYM, SYM] -> VARYING if (min_symbolic && max_symbolic) diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h index d20d0043ba3..a3f9e90699d 100644 --- a/gcc/tree-vrp.h +++ b/gcc/tree-vrp.h @@ -86,6 +86,7 @@ public: static bool supports_type_p (tree); value_range_base normalize_symbolics () const; + value_range_base normalize_addresses () const; static const unsigned int m_max_pairs = 2; bool contains_p (tree) const;