From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 0134F385C413 for ; Thu, 2 Dec 2021 17:06:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0134F385C413 Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-29-JM2KA7FuPby0ZvguB9s-wQ-1; Thu, 02 Dec 2021 12:06:37 -0500 X-MC-Unique: JM2KA7FuPby0ZvguB9s-wQ-1 Received: by mail-qt1-f200.google.com with SMTP id y25-20020ac87059000000b002a71d24c242so466144qtm.0 for ; Thu, 02 Dec 2021 09:06:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=uQhePAI8Yag7QKaLY/pqP+Hi9Dv3xEUNOv79hoDuSBk=; b=QNLezxK/zxyrr5BUF5Ei9f5XuvNvxhwMwoZerzr//fr9X3bkc2CeQp2dKYFbC6Bem0 tKVm4eU/gf1PfaXZHyq8Njca9yDfOSjvig8J3HkDljKwT+I4YwrXezU0NjeqxQPEPfeU DMpzYZ/bY6WnFgyhG1sg5bJS9BfLxh4lxD8T84E+g3qIgy2KWNuCsyBjasbP6BtvS+3A cdEFAKA8gnJtAzuVuNq41ezBmIlLcGpVJkBAu+fhI8d8HLjUonkhKdZlBYCkmz7cPRlN gQn5EBdjFtn4FHyBDs9scXZnoi6TrVfWLYQ7vFe5zQx7ySbA++sI/U4qPGITz1iH0mxB rRtQ== X-Gm-Message-State: AOAM533TfpeDSOSrsIWrvXgnxbJ9d77SfmYpIOjcRaEztFIAsnccBoVh LQvh/nVi2PKJeA8fKJR+f22gk3yVMz4VZ+pci1KS94VkkhtnPbs4268dba/5tsPtigP5wtaz5Ui F2u8ZuUOIKtQLvVXgVA== X-Received: by 2002:ac8:7f86:: with SMTP id z6mr15036344qtj.162.1638464796945; Thu, 02 Dec 2021 09:06:36 -0800 (PST) X-Google-Smtp-Source: ABdhPJxjm56hKn7zONYsI+B2iC6rB2lrb51N2NSe3pKTGOp3hduDZiFv56qRPWO/JVgGquYcbNCg4g== X-Received: by 2002:ac8:7f86:: with SMTP id z6mr15036314qtj.162.1638464796678; Thu, 02 Dec 2021 09:06:36 -0800 (PST) Received: from [192.168.1.113] ([69.165.238.126]) by smtp.gmail.com with ESMTPSA id q20sm250062qkl.53.2021.12.02.09.06.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 02 Dec 2021 09:06:36 -0800 (PST) Message-ID: Date: Thu, 2 Dec 2021 12:06:32 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0 Subject: Re: [PR103437] [committed] IRA: Process multiplication overflow in priority calculation for allocno assignments To: Jakub Jelinek Cc: Christophe Lyon , "gcc-patches@gcc.gnu.org" References: <116e765c-ea89-47f4-600f-af115dd561c3@redhat.com> <20211202140021.GH2646553@tucnak> <3bb6317f-b049-13f1-1bac-6ffd753e9685@redhat.com> <20211202142937.GI2646553@tucnak> <68638247-ddb8-b165-d24a-89a0f8155e63@redhat.com> <20211202161348.GJ2646553@tucnak> From: Vladimir Makarov In-Reply-To: <20211202161348.GJ2646553@tucnak> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.5 required=5.0 tests=BAYES_00, CTE_8BIT_MISMATCH, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Dec 2021 17:06:40 -0000 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.