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