| Message ID | alpine.LSU.2.11.1501261602400.12482@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
On Mon, Jan 26, 2015 at 04:06:11PM +0100, Richard Biener wrote: > > This is the 2nd thing I came up with after looking at PR64277. > VRP does a poor job computing value-ranges of unrolled loop IVs > thus a very simple thing to do is to factor in previous VRP results > by intersecting what VRP2 computes with recorded SSA name range infos > (that also makes errors in those more likely to pop up... :/). > > This reduces the number of array bound warnings I get from the testcase > but doesn't fix it completely. It also ends up with saner value-ranges. > > Bootstrapped and tested on x86_64-unknown-linux-gnu (with one > libstdc++ runtime failure which I am now checking if caused by the patch, > and thus likely an existing latent issue with SSA name range info). > > Ok for trunk? Or should I delay this to GCC 6? Does this work even without the other patch? What do you think about Ilya's patch to set TREE_NO_WARNING in the unrolled iterations where we had to keep the exit check? > 2015-01-26 Richard Biener <rguenther@suse.de> > > PR tree-optimization/64277 > * tree-vrp.c (update_value_range): Intersect the range with > old recorded SSA name range information. Jakub
On Mon, 26 Jan 2015, Jakub Jelinek wrote: > On Mon, Jan 26, 2015 at 04:06:11PM +0100, Richard Biener wrote: > > > > This is the 2nd thing I came up with after looking at PR64277. > > VRP does a poor job computing value-ranges of unrolled loop IVs > > thus a very simple thing to do is to factor in previous VRP results > > by intersecting what VRP2 computes with recorded SSA name range infos > > (that also makes errors in those more likely to pop up... :/). > > > > This reduces the number of array bound warnings I get from the testcase > > but doesn't fix it completely. It also ends up with saner value-ranges. > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu (with one > > libstdc++ runtime failure which I am now checking if caused by the patch, > > and thus likely an existing latent issue with SSA name range info). > > > > Ok for trunk? Or should I delay this to GCC 6? > > Does this work even without the other patch? Yes, I've actually developed 2/2 first. The other patch only ever emits more warnings... > What do you think about Ilya's patch to set TREE_NO_WARNING in the unrolled > iterations where we had to keep the exit check? I don't like it too much - it papers over the real issue and prevents valid warnings from being emitted. Maybe we can set TREE_NO_WARNING on the last iteration (that is the only one we usually end up warning on - like after this patch). There are also many dups - regressions in 4.8 I belive where we warn for the last iteration in an unrolled loop. Richard.
On Mon, Jan 26, 2015 at 04:18:32PM +0100, Richard Biener wrote: > > > Ok for trunk? Or should I delay this to GCC 6? > > > > Does this work even without the other patch? > > Yes, I've actually developed 2/2 first. The other patch only ever > emits more warnings... Then it probably should be ok. I'm really afraid of emitting more warnings with such high false positive rate now. > > What do you think about Ilya's patch to set TREE_NO_WARNING in the unrolled > > iterations where we had to keep the exit check? > > I don't like it too much - it papers over the real issue and prevents > valid warnings from being emitted. Maybe we can set TREE_NO_WARNING on > the last iteration (that is the only one we usually end up warning > on - like after this patch). There are also many dups - regressions > in 4.8 I belive where we warn for the last iteration in an unrolled loop. Do we check only the last iteration? Won't we warn about other iterations too? -Warray-bounds have plenty of warnings in dead jump threaded code that the compiler can't prove is dead etc. IMHO the warning should be done in VRP1 only. Jakub
On Mon, 26 Jan 2015, Jakub Jelinek wrote: > On Mon, Jan 26, 2015 at 04:18:32PM +0100, Richard Biener wrote: > > > > Ok for trunk? Or should I delay this to GCC 6? > > > > > > Does this work even without the other patch? > > > > Yes, I've actually developed 2/2 first. The other patch only ever > > emits more warnings... > > Then it probably should be ok. I'm really afraid of emitting more warnings > with such high false positive rate now. > > > > What do you think about Ilya's patch to set TREE_NO_WARNING in the unrolled > > > iterations where we had to keep the exit check? > > > > I don't like it too much - it papers over the real issue and prevents > > valid warnings from being emitted. Maybe we can set TREE_NO_WARNING on > > the last iteration (that is the only one we usually end up warning > > on - like after this patch). There are also many dups - regressions > > in 4.8 I belive where we warn for the last iteration in an unrolled loop. > > Do we check only the last iteration? Won't we warn about other iterations > too? -Warray-bounds have plenty of warnings in dead jump threaded code that > the compiler can't prove is dead etc. Sure - but for unrolling int a[2]; for (int i = 0; i < 5; i++) a[i] = i; I'd like to see warnings and we only warn if we unroll this because the value-range of i includes indexes that are valid. > IMHO the warning should be done in VRP1 only. Yeah, I agree - but I remember that people wanted the extra stuff from VRP2 (just quickly checked that gcc.dg/Warray-bounds* doesn't regress with disabling VRP2). So - do we want to disable array bound warnings for VRP2? I'd be happy to approve of that and it will most certainly fix all of the recent (4.8+) regressions related to loop peeling. Thanks, Richard.
On Mon, Jan 26, 2015 at 04:28:02PM +0100, Richard Biener wrote: > Sure - but for unrolling > > int a[2]; > for (int i = 0; i < 5; i++) > a[i] = i; > > I'd like to see warnings and we only warn if we unroll this because > the value-range of i includes indexes that are valid. Don't we warn here for -Waggressive-loop-optimizations already? > > IMHO the warning should be done in VRP1 only. > > Yeah, I agree - but I remember that people wanted the extra stuff > from VRP2 (just quickly checked that gcc.dg/Warray-bounds* doesn't > regress with disabling VRP2). > > So - do we want to disable array bound warnings for VRP2? I'd be > happy to approve of that and it will most certainly fix all of the > recent (4.8+) regressions related to loop peeling. That would be my preference, if not many people complain about that. People can use -fsanitize=undefined if they want more accurate and detailed out of bounds checking anyway, and/or -fsanitize=address. Jakub
On Mon, 26 Jan 2015, Jakub Jelinek wrote: > On Mon, Jan 26, 2015 at 04:18:32PM +0100, Richard Biener wrote: > > > > Ok for trunk? Or should I delay this to GCC 6? > > > > > > Does this work even without the other patch? > > > > Yes, I've actually developed 2/2 first. The other patch only ever > > emits more warnings... > > Then it probably should be ok. I'm really afraid of emitting more warnings > with such high false positive rate now. As the patch also mitigates some of the code bloat we get with the complete peeling (regression against 4.7) I have installed it. It's also the easiest vehicle to verify range-info is not broken by passes between vrp1 and vrp2. Thanks, Richard.
Richard Biener wrote: > On Mon, 26 Jan 2015, Jakub Jelinek wrote: > > Then it probably should be ok. I'm really afraid of emitting more warnings > > with such high false positive rate now. > > As the patch also mitigates some of the code bloat we get with > the complete peeling (regression against 4.7) I have installed it. > It's also the easiest vehicle to verify range-info is not broken > by passes between vrp1 and vrp2. You could make warnings appear only for warn_array_bounds > 1 if there are concerns about false positives. For what it's worth, I tested the old version of both patches on one of my projects (mostly numerical algorithms) and it did not produce additional warnings. I really appreciate all improvements in this area. Martin
On Mon, Jan 26, 2015 at 7:06 AM, Richard Biener <rguenther@suse.de> wrote: > > This is the 2nd thing I came up with after looking at PR64277. > VRP does a poor job computing value-ranges of unrolled loop IVs > thus a very simple thing to do is to factor in previous VRP results > by intersecting what VRP2 computes with recorded SSA name range infos > (that also makes errors in those more likely to pop up... :/). > > This reduces the number of array bound warnings I get from the testcase > but doesn't fix it completely. It also ends up with saner value-ranges. > > Bootstrapped and tested on x86_64-unknown-linux-gnu (with one > libstdc++ runtime failure which I am now checking if caused by the patch, > and thus likely an existing latent issue with SSA name range info). > > Ok for trunk? Or should I delay this to GCC 6? > > Thanks, > Richard. > > 2015-01-26 Richard Biener <rguenther@suse.de> > > PR tree-optimization/64277 > * tree-vrp.c (update_value_range): Intersect the range with > old recorded SSA name range information. > This caused: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69606 H.J.
Index: gcc/tree-vrp.c =================================================================== --- gcc/tree-vrp.c (revision 220107) +++ gcc/tree-vrp.c (working copy) @@ -847,6 +847,23 @@ update_value_range (const_tree var, valu value_range_t *old_vr; bool is_new; + /* If there is a value-range on the SSA name from earlier analysis + factor that in. */ + if (INTEGRAL_TYPE_P (TREE_TYPE (var))) + { + wide_int min, max; + value_range_type rtype = get_range_info (var, &min, &max); + if (rtype == VR_RANGE || rtype == VR_ANTI_RANGE) + { + value_range_d nr; + nr.type = rtype; + nr.min = wide_int_to_tree (TREE_TYPE (var), min); + nr.max = wide_int_to_tree (TREE_TYPE (var), max); + nr.equiv = NULL; + vrp_intersect_ranges (new_vr, &nr); + } + } + /* Update the value range, if necessary. */ old_vr = get_value_range (var); is_new = old_vr->type != new_vr->type