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

Reply via email to