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.
That would be
#ifdef __has_builtin
#if __has_builtin(__builtin_smul_overflow)
#define HAS_SMUL_OVERFLOW
#endif
#endif
#ifdef HAS_SMUL_OVERFLOW
if (__builtin_smul_overflow (mult, diff, &priority)
|| 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 int wider"
"than half of long long");
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
Why priority <= -INT_MAX in the first case though,
shouldn't that be < -INT_MAX ?
My thought was to avoid 'always false' warning for non two's compliment
binary representation targets. As I remember C++17 started to require
only two-compliment integers. If we require to use only c++17 and
upper, then probably it is better to fix it.
In any case, I feel these details are not my area of expertise. If you
believe I should do these changes, please confirm that you want these
changes and I'll definitely do this. Thank you, Jakub.
