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

[Vladimir Makarov <vmakarov@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]


On 2021-12-02 12:21, Vladimir Makarov via Gcc-patches wrote:
>
> On 2021-12-02 12:06, Vladimir Makarov wrote:
>>
>>
>> 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.
>
>
Here is one more patch I've committed.  Jakub, thank your for the 
discussion and your patience.


[-- Attachment #2: pr103437-3.patch --]
[-- Type: text/x-patch, Size: 2627 bytes --]

commit a72b8f376a176c620f1c1c684f2eee2016e6b4c3
Author: Vladimir N. Makarov <vmakarov@redhat.com>
Date:   Thu Dec 2 12:31:28 2021 -0500

    [PR103437] Make backup code for overflow conditional
    
    Switch off long long variant overflow code by preprocessor if the
    build compiler has __builtin_smul_overflow.
    
    gcc/ChangeLog:
            PR rtl-optimization/103437
            * ira-color.c (setup_allocno_priorities): Switch off backup code
            for overflow if compiler has __builtin_smul_overflow.  Use <
            for comparison with -INT_MAX.

diff --git a/gcc/ira-color.c b/gcc/ira-color.c
index 3b19a58e1f0..a1b02776e77 100644
--- a/gcc/ira-color.c
+++ b/gcc/ira-color.c
@@ -2797,7 +2797,6 @@ 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;
@@ -2810,27 +2809,27 @@ setup_allocno_priorities (ira_allocno_t *consideration_allocnos, int n)
       ira_assert (mult >= 0);
       mult *= ira_reg_class_max_nregs[ALLOCNO_CLASS (a)][ALLOCNO_MODE (a)];
       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;
+#define HAS_SMUL_OVERFLOW
+#endif
+#endif
+      /* Multiplication can overflow for very large functions.
+	 Check the overflow and constrain the result if necessary: */
+#ifdef HAS_SMUL_OVERFLOW
       if (__builtin_smul_overflow (mult, diff, &priority)
-	  || priority <= -INT_MAX)
+	  || priority < -INT_MAX)
 	priority = diff >= 0 ? INT_MAX : -INT_MAX;
+#else
+      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;
 #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;
-	}
       allocno_priorities[ALLOCNO_NUM (a)] = priority;
       if (priority < 0)
 	priority = -priority;