Re: [PR103437] [committed] IRA: Process multiplication overflow in priority calculation for allocno assignments

[Vladimir Makarov <vmakarov@redhat.com>]

On 2021-12-02 12:06, Vladimir Makarov wrote:
>
> On 2021-12-02 11:13, Jakub Jelinek wrote:
>> On Thu, Dec 02, 2021 at 11:03:46AM -0500, Vladimir Makarov wrote:
>>> --- a/gcc/ira-color.c
>>> +++ b/gcc/ira-color.c
>>> @@ -2797,6 +2797,7 @@ static void
>>>   setup_allocno_priorities (ira_allocno_t *consideration_allocnos, 
>>> int n)
>>>   {
>>>     int i, length, nrefs, priority, max_priority, mult, diff;
>>> +  bool overflow_backup_p = true;
>>>     ira_allocno_t a;
>>>       max_priority = 0;
>>> @@ -2811,9 +2812,25 @@ setup_allocno_priorities (ira_allocno_t 
>>> *consideration_allocnos, int n)
>>>         diff = ALLOCNO_MEMORY_COST (a) - ALLOCNO_CLASS_COST (a);
>>>         /* Multiplication can overflow for very large functions.
>>>        Check the overflow and constrain the result if necessary: */
>>> +#ifdef __has_builtin
>>> +#if __has_builtin(__builtin_smul_overflow)
>>> +      overflow_backup_p = false;
>>>         if (__builtin_smul_overflow (mult, diff, &priority)
>>>         || priority <= -INT_MAX)
>>>       priority = diff >= 0 ? INT_MAX : -INT_MAX;
>>> +#endif
>>> +#endif
>>> +      if (overflow_backup_p)
>>> +    {
>>> +      static_assert
>>> +        (sizeof (long long) >= 2 * sizeof (int),
>>> +         "overflow code does not work for such int and long long 
>>> sizes");
>>> +      long long priorityll = (long long) mult * diff;
>>> +      if (priorityll < -INT_MAX || priorityll > INT_MAX)
>>> +        priority = diff >= 0 ? INT_MAX : -INT_MAX;
>>> +      else
>>> +        priority = priorityll;
>>> +    }
> So simple problem and so many details :)
>> This will require that long long is at least twice as large as int
>> everywhere, I thought you wanted to do that only when
>> __builtin_smul_overflow isn't available.
>
> That is not critical as GCC and probably all others C++ compiler 
> support only targets with this assertion.  I guess it is better to 
> find this problem earlier on targets (if any) where it is not true 
> *independently* on used compiler.
>
> So it is difficult for me to know what is better.  Probably for 
> GCC/Clang oriented world, your variant is better as it permits to 
> compile the code by GCC even on targets where the assertion is false.
>
>
After some more considerations, I think you are right and the backup 
code should be conditional.  Because otherwise, there is no sense to use 
code with __builtin_smul_overflow.  I'll do the changes.