Re: locating unsigned type for non-standard precision

[Georg-Johann Lay <avr at gjlay dot de>]

Georg-Johann Lay wrote:
> Richard Guenther wrote:
>> On Tue, Apr 24, 2012 at 7:24 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
>>> Richard Guenther wrote:
>>>
>>>>> I've run into another issue supporting a 20-bit integer for which I'd
>>>>> appreciate a hint.  With this code:
>>>>>
>>>>>   typedef long int __attribute__((__a20__)) int20_t;
>>>>>   int20_t xi;
>>>>>   int20_t addit () { xi += 0x54321L; }
>>>>>
>>>>> xi ends up in mode PSImode, which is a MODE_PARTIAL_INT with 20 bits of
>>>>> precision and 32-bit width.
>>>>>
>>>>> convert() notices that, because the constant in the add expression is
>>>>> SImode, there's an SImode add being truncated to a PSImode result, and
>>>>> pushes the truncation down into the operands.
>>>>>
>>>>> The problem is this ends up in convert_to_integer, which detects that the
>>>>> signed operation might overflow so calls unsigned_type_for() to get the
>>>>> unsigned variant.
>>>>>
>>>>> Unfortunately, this ends up in c_common_type_for_size(), which knows nothing
>>>>> about PSImode, and returns an unsigned type with 32 bits of precision when
>>>>> asked for one with 20 bits of precision.
>>>> That's expected - this function returns a type that is suitable for holding all
>>>> values, not a type that has necessarily matching precision.  If the caller
>>>> wants such type it needs to verify what the function returned.  Which seems
>>>> to me to be the correct fix for this (premature) optimization in
>>>> convert_to_integer.
>>>>
>>>> Richard.
>>>>
>>>>>   The expression is rewritten with
>>>>> the 32-bit constant integer recast to the 32-bit unsigned type (instead
>>>>> of the 20-bit one it might have used), and infinite recursion results.
>>> This is already filed as
>>>
>>> http://gcc.gnu.org/PR51527
>>>
>>> It works with 4.8 trunk but crashes with 4.7.
>>> Did not yet track what changes made it work with 4.8, though.
>>> Unfortunately, noone remembers :-(
>>>
>>> http://gcc.gnu.org/ml/gcc/2012-03/msg00440.html
>> I have done changes in this area, trying to remove the type_for_size langhook.
>>
>> Richard.
> 
> One apparent change is tree.c:signed_or_unsigned_type_for
> 
> that changed from 4.7:
> 
> tree
> signed_or_unsigned_type_for (int unsignedp, tree type)
> {
>   tree t = type;
>   if (POINTER_TYPE_P (type))
>     {
>       /* If the pointer points to the normal address space, use the
> 	 size_type_node.  Otherwise use an appropriate size for the pointer
> 	 based on the named address space it points to.  */
>       if (!TYPE_ADDR_SPACE (TREE_TYPE (t)))
> 	t = size_type_node;
>       else
> 	return lang_hooks.types.type_for_size (TYPE_PRECISION (t), unsignedp);
>     }
> 
>   if (!INTEGRAL_TYPE_P (t) || TYPE_UNSIGNED (t) == unsignedp)
>     return t;
> 
>   return lang_hooks.types.type_for_size (TYPE_PRECISION (t), unsignedp);
> }
> 
> to 4.8:
> 
> tree
> signed_or_unsigned_type_for (int unsignedp, tree type)
> {
>   if (TREE_CODE (type) == INTEGER_TYPE && TYPE_UNSIGNED (type) == unsignedp)
>     return type;
> 
>   if (!INTEGRAL_TYPE_P (type)
>       && !POINTER_TYPE_P (type))
>     return NULL_TREE;
> 
>   return build_nonstandard_integer_type (TYPE_PRECISION (type), unsignedp);
> }
> 
> at 2012-03-12
> 
> http://gcc.gnu.org/viewcvs?view=revision&revision=185226
> 
> Is this appropriate to backport?
> 
> Or is the preferred solution to override lang_hooks.types.type_for_size in the
> backend, if applicable?

Now I have this piece of code that works for 4.7.

I don't like it much, but if there are no plans to otherwise fix PR51527,
I'd make a patch and propose it as target-specific work-around.

/* Some auxiliary stuff for the AVR-specific built-in 24-bit scalar integer
   types __int24 and __uint24 to be used in `avr_init_builtins'.  */

static GTY(()) tree avr_int24_type;
static GTY(()) tree avr_uint24_type;


/* Implement `LANG_HOOKS_TYPE_FOR_SIZE'.  */
/* To avoid infinite recursion in `convert_to_integer' as
   reported in PR c/51527.  */

static tree (*avr_default_type_for_size)(unsigned int, int);

static tree
avr_type_for_size (unsigned int bits, int unsignedp)
{
  if (bits == 24)
    return unsignedp ? avr_uint24_type : avr_int24_type;

  return avr_default_type_for_size (bits, unsignedp);
}


static void
avr_init_builtin_int24 (void)
{
  avr_int24_type  = make_signed_type (GET_MODE_BITSIZE (PSImode));
  avr_uint24_type = make_unsigned_type (GET_MODE_BITSIZE (PSImode));

  lang_hooks.types.register_builtin_type (avr_int24_type, "__int24");
  lang_hooks.types.register_builtin_type (avr_uint24_type, "__uint24");

  /* Hook-in to avoid infinite recursion in `convert_to_integer' as
     reported in PR c/51527.  */

  avr_default_type_for_size = lang_hooks.types.type_for_size;
  lang_hooks.types.type_for_size = avr_type_for_size;
}


A proper, target-independent fix would be great because it is the right place
and because Peter has the same problem.

Johann

>>>>> Is it proper for c_common_type_for_size() to know about partial int modes
>>>>> and return the best one available?  Is there a hook that would allow me to
>>>>> do this customized in my back-end?  Or is there another way to get
>>>>> unsigned_type_for() to return the "right" type for an unusual integer
>>>>> precision?
>>>>>
>>>>> Thanks.
>>>>>
>>>>> Peter