From patchwork Fri Dec 12 15:47:02 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 420531 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 553821400B7 for ; Sat, 13 Dec 2014 02:47:17 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:references:date:in-reply-to:message-id :mime-version:content-type:content-transfer-encoding; q=dns; s= default; b=oE8jQNJGy2ZvbSYIUrztyVwLA1CMxgBSsPEfES/uEBuhdn78jgt37 L1PgYeIbPzdPbNRTPBpF5MXUeV6KpRxRyBYoCj9Pt8xKaSdauzBkPxR3EDguNt14 1RZRbPrkiG/WbLeNUP0wbDzi+Urcp6/WzC7CrgVVyh9FkUmLnkieNA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:references:date:in-reply-to:message-id :mime-version:content-type:content-transfer-encoding; s=default; bh=6wLBQkWxA9E8aWJLWJhYYRMerbE=; b=qKbM736/DRr9v+B2pjbrZKxkrbSk RuWU6gIAMvnMskdQRJJpSbKLHHVq+Bm8a9gtM50PjkoO5HjSdOVQeW3k5pISRQUn F6ByouaaruUW6G6FHxxqioWMyyOMVspGuvebJX/tt21P+VKoKfRCOiH49g+qnwqQ Z3vtKTvQPA3wLzc= Received: (qmail 10889 invoked by alias); 12 Dec 2014 15:47:09 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 10871 invoked by uid 89); 12 Dec 2014 15:47:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 X-HELO: service87.mimecast.com Received: from service87.mimecast.com (HELO service87.mimecast.com) (91.220.42.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 12 Dec 2014 15:47:05 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by service87.mimecast.com; Fri, 12 Dec 2014 15:47:02 +0000 Received: from localhost ([10.1.255.212]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Fri, 12 Dec 2014 15:47:02 +0000 From: Richard Sandiford To: Richard Biener Mail-Followup-To: Richard Biener , GCC Patches , richard.sandiford@arm.com Cc: GCC Patches Subject: Re: PR64182: Fix rounding division and modulus References: <87sigm8j9d.fsf@e105548-lin.cambridge.arm.com> Date: Fri, 12 Dec 2014 15:47:02 +0000 In-Reply-To: (Richard Biener's message of "Thu, 11 Dec 2014 12:40:12 +0000") Message-ID: <87oar898g9.fsf@e105548-lin.cambridge.arm.com> User-Agent: Gnus/5.130012 (Ma Gnus v0.12) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-MC-Unique: 114121215470203301 Richard Biener writes: > On Thu, Dec 11, 2014 at 1:26 PM, Richard Sandiford > wrote: >> As pointed out in PR 64182, wide-int rounded division gets the >> ties-away-from-zero case wrong for odd-numbered dividends, while >> double_int gets the unsigned case wrong by unconditionally treating >> a dividend or remainder with the top bit set as negative. As Jakub >> says, the test used in double_int might also have overflow problems. >> >> This patch uses: >> >> abs (remainder) >= abs (dividend) - abs (remainder) >> >> for both wide-int and double_int and fixes the unsigned case in double_int. >> I didn't know how to test the double_int change using input code so >> resorted to doing some double_int arithmetic at the start of main. >> >> Thanks to Joseph for the testcase. >> >> Tested on x86_64-linux-gnu. OK to install? > > Can you add a testcase? You can follow the gcc.dg/plugin/sreal_plugin.c > example, maybe even make it a generic host_test_plugin.c with separate > files containing the actual tests. > > Otherwise ok. Ah, hadn't realised we could do that. Much neater than changing main :-) Here's what I committed after retesting on x86_64-linux-gnu. As well as the testcase, I changed x - y to the more general wi::sub (x, y). Thanks, Richard gcc/ PR middle-end/64182 * wide-int.h (wi::div_round, wi::mod_round): Fix rounding of tied cases. * double-int.c (div_and_round_double): Fix handling of unsigned cases. Use same rounding approach as wide-int.h. gcc/testsuite/ 2014-xx-xx Richard Sandiford Joseph Myers PR middle-end/64182 * gcc.dg/plugin/wide-int-test-1.c, gcc.dg/plugin/wide-int_plugin.c: New test. * gcc.dg/plugin/plugin.exp: Register it. * gnat.dg/round_div.adb: New test. Index: gcc/wide-int.h =================================================================== --- gcc/wide-int.h 2014-12-11 14:32:17.708138315 +0000 +++ gcc/wide-int.h 2014-12-11 14:32:17.704138366 +0000 @@ -2616,8 +2616,8 @@ wi::div_round (const T1 &x, const T2 &y, { if (sgn == SIGNED) { - if (wi::ges_p (wi::abs (remainder), - wi::lrshift (wi::abs (y), 1))) + WI_BINARY_RESULT (T1, T2) abs_remainder = wi::abs (remainder); + if (wi::geu_p (abs_remainder, wi::sub (wi::abs (y), abs_remainder))) { if (wi::neg_p (x, sgn) != wi::neg_p (y, sgn)) return quotient - 1; @@ -2627,7 +2627,7 @@ wi::div_round (const T1 &x, const T2 &y, } else { - if (wi::geu_p (remainder, wi::lrshift (y, 1))) + if (wi::geu_p (remainder, wi::sub (y, remainder))) return quotient + 1; } } @@ -2784,8 +2784,8 @@ wi::mod_round (const T1 &x, const T2 &y, { if (sgn == SIGNED) { - if (wi::ges_p (wi::abs (remainder), - wi::lrshift (wi::abs (y), 1))) + WI_BINARY_RESULT (T1, T2) abs_remainder = wi::abs (remainder); + if (wi::geu_p (abs_remainder, wi::sub (wi::abs (y), abs_remainder))) { if (wi::neg_p (x, sgn) != wi::neg_p (y, sgn)) return remainder + y; @@ -2795,7 +2795,7 @@ wi::mod_round (const T1 &x, const T2 &y, } else { - if (wi::geu_p (remainder, wi::lrshift (y, 1))) + if (wi::geu_p (remainder, wi::sub (y, remainder))) return remainder - y; } } Index: gcc/double-int.c =================================================================== --- gcc/double-int.c 2014-12-11 14:32:17.708138315 +0000 +++ gcc/double-int.c 2014-12-11 14:32:17.700138416 +0000 @@ -569,24 +569,23 @@ div_and_round_double (unsigned code, int { unsigned HOST_WIDE_INT labs_rem = *lrem; HOST_WIDE_INT habs_rem = *hrem; - unsigned HOST_WIDE_INT labs_den = lden, ltwice; - HOST_WIDE_INT habs_den = hden, htwice; + unsigned HOST_WIDE_INT labs_den = lden, lnegabs_rem, ldiff; + HOST_WIDE_INT habs_den = hden, hnegabs_rem, hdiff; /* Get absolute values. */ - if (*hrem < 0) + if (!uns && *hrem < 0) neg_double (*lrem, *hrem, &labs_rem, &habs_rem); - if (hden < 0) + if (!uns && hden < 0) neg_double (lden, hden, &labs_den, &habs_den); - /* If (2 * abs (lrem) >= abs (lden)), adjust the quotient. */ - mul_double ((HOST_WIDE_INT) 2, (HOST_WIDE_INT) 0, - labs_rem, habs_rem, <wice, &htwice); + /* If abs(rem) >= abs(den) - abs(rem), adjust the quotient. */ + neg_double (labs_rem, habs_rem, &lnegabs_rem, &hnegabs_rem); + add_double (labs_den, habs_den, lnegabs_rem, hnegabs_rem, + &ldiff, &hdiff); - if (((unsigned HOST_WIDE_INT) habs_den - < (unsigned HOST_WIDE_INT) htwice) - || (((unsigned HOST_WIDE_INT) habs_den - == (unsigned HOST_WIDE_INT) htwice) - && (labs_den <= ltwice))) + if (((unsigned HOST_WIDE_INT) habs_rem + > (unsigned HOST_WIDE_INT) hdiff) + || (habs_rem == hdiff && labs_rem >= ldiff)) { if (quo_neg) /* quo = quo - 1; */ Index: gcc/testsuite/gcc.dg/plugin/wide-int-test-1.c =================================================================== --- /dev/null 2014-11-19 08:41:51.310561007 +0000 +++ gcc/testsuite/gcc.dg/plugin/wide-int-test-1.c 2014-12-11 14:32:17.704138366 +0000 @@ -0,0 +1,9 @@ +/* Test that pass is inserted and invoked once. */ +/* { dg-do compile } */ +/* { dg-options "-O" } */ + +int +main (int argc, char **argv) +{ + return 0; +} Index: gcc/testsuite/gcc.dg/plugin/wide-int_plugin.c =================================================================== --- /dev/null 2014-11-19 08:41:51.310561007 +0000 +++ gcc/testsuite/gcc.dg/plugin/wide-int_plugin.c 2014-12-11 14:32:27.792013373 +0000 @@ -0,0 +1,46 @@ +#include "config.h" +#include "gcc-plugin.h" +#include "system.h" +#include "coretypes.h" +#include "tree.h" + +int plugin_is_GPL_compatible; + +static void +test_double_int_round_udiv (void) +{ + double_int dmin = { 0, HOST_WIDE_INT_MIN }; + double_int dmax = { -1, HOST_WIDE_INT_MAX }; + double_int dnegone = { -1, -1 }; + double_int mod, div; + div = dmin.udivmod (dnegone, ROUND_DIV_EXPR, &mod); + if (div.low != 1 || div.high != 0 + || mod.low != 1 || mod.high != HOST_WIDE_INT_MIN) + abort (); + div = dmax.udivmod (dnegone, ROUND_DIV_EXPR, &mod); + if (div.low != 0 || div.high != 0 + || mod.low != dmax.low || mod.high != dmax.high) + abort (); +} + +static void +test_wide_int_round_sdiv (void) +{ + if (wi::ne_p (wi::div_round (2, 3, SIGNED), 1)) + abort (); + if (wi::ne_p (wi::div_round (1, 3, SIGNED), 0)) + abort (); + if (wi::ne_p (wi::mod_round (2, 3, SIGNED), -1)) + abort (); + if (wi::ne_p (wi::mod_round (1, 3, SIGNED), 1)) + abort (); +} + +int +plugin_init (struct plugin_name_args *plugin_info, + struct plugin_gcc_version *version) +{ + test_double_int_round_udiv (); + test_wide_int_round_sdiv (); + return 0; +} Index: gcc/testsuite/gcc.dg/plugin/plugin.exp =================================================================== --- gcc/testsuite/gcc.dg/plugin/plugin.exp 2014-12-11 14:32:17.708138315 +0000 +++ gcc/testsuite/gcc.dg/plugin/plugin.exp 2014-12-11 14:32:17.704138366 +0000 @@ -62,6 +62,7 @@ set plugin_test_list [list \ { sreal_plugin.c sreal-test-1.c } \ { start_unit_plugin.c start_unit-test-1.c } \ { finish_unit_plugin.c finish_unit-test-1.c } \ + { wide-int_plugin.c wide-int-test-1.c } \ ] foreach plugin_test $plugin_test_list { Index: gcc/testsuite/gnat.dg/round_div.adb =================================================================== --- /dev/null 2014-11-19 08:41:51.310561007 +0000 +++ gcc/testsuite/gnat.dg/round_div.adb 2014-12-11 14:32:17.700138416 +0000 @@ -0,0 +1,17 @@ +-- { dg-do run } +-- { dg-options "-O3" } +procedure Round_Div is + type Fixed is delta 1.0 range -2147483648.0 .. 2147483647.0; + A : Fixed := 1.0; + B : Fixed := 3.0; + C : Integer; + function Divide (X, Y : Fixed) return Integer is + begin + return Integer (X / Y); + end; +begin + C := Divide (A, B); + if C /= 0 then + raise Program_Error; + end if; +end Round_Div;