[ping] Fix PR debug/66728

On Nov 4, 2015, at 4:15 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> I wonder if we'll manage to to get mode_for_size return BLKmode
> in case of an original mode that was not of a size multiple of
> HOST_BITS_PER_WIDE_INT (and that's host dependent even…).

> We probably should use smallest_mode_for_size on a precision
> derived from the value

Once we want to go stomping around the value, we might as well just do everything.  I prefer this version over one that has a call to assert.  I thought about creating a helper function, but since there is only 1 client for it, and since it was only 4 lines, I didn’t create one.  I’d propose deferring creation until we have more clients.  The generated dwarf remains fixed, as expected.

> Please use gcc_checking_assert here.

Done.  My test suite run with the assert did finish, and on x86_64 linux, it was never hit.

Any other issues you can spot?

Mike Stump <mikestump@comcast.net> writes:
> Index: dwarf2out.c
> ===================================================================
> --- dwarf2out.c	(revision 229720)
> +++ dwarf2out.c	(working copy)
> @@ -15593,8 +15593,13 @@
>        return true;
>  
>      case CONST_WIDE_INT:
> -      add_AT_wide (die, DW_AT_const_value,
> -		   std::make_pair (rtl, GET_MODE (rtl)));
> +      {
> +	wide_int w1 = std::make_pair (rtl, MAX_MODE_INT);
> +	int prec = MIN (wi::min_precision (w1, UNSIGNED),
> +			(unsigned int)CONST_WIDE_INT_NUNITS (rtl) * HOST_BITS_PER_WIDE_INT);
> +	wide_int w = wide_int::from (w1, prec, UNSIGNED);
> +	add_AT_wide (die, DW_AT_const_value, w);
> +      }
>        return true;
>  
>      case CONST_DOUBLE:

Setting the precision based on CONST_WIDE_INT_NUNITS means that
we might end up with two different precisions for two values of
the same variable.  E.g. for a 192-bit type, 1<<64 would be given
a precision of 128 (because it needs two HWIs to store) but 1<<128
would be given a precision of 192 (because it needs three HWIs to store).
We could then abort when comparing them for equality, since equality
needs both integers to have the same precision.  E.g. from same_dw_val_p:

    case dw_val_class_wide_int:
      return *v1->v.val_wide == *v2->v.val_wide;

Thanks,
Richard

On Nov 4, 2015, at 12:50 PM, Richard Sandiford <richard.sandiford@arm.com> wrote:

> Mike Stump <mikestump@comcast.net> writes:
>> Index: dwarf2out.c
>> ===================================================================
>> --- dwarf2out.c	(revision 229720)
>> +++ dwarf2out.c	(working copy)
>> @@ -15593,8 +15593,13 @@
>>       return true;
>> 
>>     case CONST_WIDE_INT:
>> -      add_AT_wide (die, DW_AT_const_value,
>> -		   std::make_pair (rtl, GET_MODE (rtl)));
>> +      {
>> +	wide_int w1 = std::make_pair (rtl, MAX_MODE_INT);
>> +	int prec = MIN (wi::min_precision (w1, UNSIGNED),
>> +			(unsigned int)CONST_WIDE_INT_NUNITS (rtl) * HOST_BITS_PER_WIDE_INT);
>> +	wide_int w = wide_int::from (w1, prec, UNSIGNED);
>> +	add_AT_wide (die, DW_AT_const_value, w);
>> +      }
>>       return true;
>> 
>>     case CONST_DOUBLE:
> 
> Setting the precision based on CONST_WIDE_INT_NUNITS means that
> we might end up with two different precisions for two values of
> the same variable.  E.g. for a 192-bit type, 1<<64 would be given
> a precision of 128 (because it needs two HWIs to store) but 1<<128
> would be given a precision of 192 (because it needs three HWIs to store).
> We could then abort when comparing them for equality, since equality
> needs both integers to have the same precision.  E.g. from same_dw_val_p:
> 
>    case dw_val_class_wide_int:
>      return *v1->v.val_wide == *v2->v.val_wide;

Yeah, seems like we should have a v1.prec == v2.prec && on that.  The bad news, there are four of them that are like this.  The good news, 3 of them are location operands, and I don’t think they can hit for a very long time.  I think this is an oversight from the double_int version of the code where we just check the 128 bits for equality.  We can see if Richard wants to weigh in.  I think I’d just pre-approve the change, though, I think a helper to perform mixed equality testing would be the way to go as there are 4 of them, and I pretty sure they should all use the mixed version.  Though, maybe the location list versions are never mixed.  If they aren’t, then there is only 1 client, so, I’d just do the precision test inline.  Anyone able to comment on the location list aspect of this?

On Thu, Nov 5, 2015 at 12:45 AM, Mike Stump <mikestump@comcast.net> wrote:
>
> On Nov 4, 2015, at 12:50 PM, Richard Sandiford <richard.sandiford@arm.com> wrote:
>
>> Mike Stump <mikestump@comcast.net> writes:
>>> Index: dwarf2out.c
>>> ===================================================================
>>> --- dwarf2out.c      (revision 229720)
>>> +++ dwarf2out.c      (working copy)
>>> @@ -15593,8 +15593,13 @@
>>>       return true;
>>>
>>>     case CONST_WIDE_INT:
>>> -      add_AT_wide (die, DW_AT_const_value,
>>> -               std::make_pair (rtl, GET_MODE (rtl)));
>>> +      {
>>> +    wide_int w1 = std::make_pair (rtl, MAX_MODE_INT);
>>> +    int prec = MIN (wi::min_precision (w1, UNSIGNED),
>>> +                    (unsigned int)CONST_WIDE_INT_NUNITS (rtl) * HOST_BITS_PER_WIDE_INT);
>>> +    wide_int w = wide_int::from (w1, prec, UNSIGNED);
>>> +    add_AT_wide (die, DW_AT_const_value, w);
>>> +      }
>>>       return true;
>>>
>>>     case CONST_DOUBLE:
>>
>> Setting the precision based on CONST_WIDE_INT_NUNITS means that
>> we might end up with two different precisions for two values of
>> the same variable.  E.g. for a 192-bit type, 1<<64 would be given
>> a precision of 128 (because it needs two HWIs to store) but 1<<128
>> would be given a precision of 192 (because it needs three HWIs to store).
>> We could then abort when comparing them for equality, since equality
>> needs both integers to have the same precision.  E.g. from same_dw_val_p:
>>
>>    case dw_val_class_wide_int:
>>      return *v1->v.val_wide == *v2->v.val_wide;
>
> Yeah, seems like we should have a v1.prec == v2.prec && on that.  The bad news, there are four of them that are like this.  The good news, 3 of them are location operands, and I don’t think they can hit for a very long time.  I think this is an oversight from the double_int version of the code where we just check the 128 bits for equality.  We can see if Richard wants to weigh in.  I think I’d just pre-approve the change, though, I think a helper to perform mixed equality testing would be the way to go as there are 4 of them, and I pretty sure they should all use the mixed version.  Though, maybe the location list versions are never mixed.  If they aren’t, then there is only 1 client, so, I’d just do the precision test inline.  Anyone able to comment on the location list aspect of this?

No idea on location lists but maybe this means we should just use the
maximum supported integer mode for CONST_WIDE_INTs?