From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com [IPv6:2607:f8b0:4864:20::1036]) by sourceware.org (Postfix) with ESMTPS id 3E3C13858417 for ; Mon, 23 Aug 2021 12:50:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3E3C13858417 Received: by mail-pj1-x1036.google.com with SMTP id u13-20020a17090abb0db0290177e1d9b3f7so18618655pjr.1 for ; Mon, 23 Aug 2021 05:50:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=C+ouUPEc8O+l6vJTFCO8eQhniq7aEenuE/6moDlZTjU=; b=QFOqPi4IZNfqdHnypxOsBzG5JdykGEmYDXlzbnZT59wqE8I7YJNI/XP0FBLyAPNYXW X7MaVRojXt8KTBiVN7uFDfVbMJ4QOo7SfMaHkUE5KsIfSfGMH2R6d4ahCV75EiSdN63e 8VYg1uj86oLaK31NTJVI8CVnVoOiCNNT1JvzDUB09XRdBwlv5Ff9BI/6pGwoD+qWqfWN F7Ci2MTR2o3howA5zIvUeHa5Zm2QGojFi8PKkyPTtpont+QLSpmC05f6dXIq2axclPGU DYZnVZ5HjtgUE+WFReEFhKmtnF572a7L5UHpzvpj9iuS4rbky7uD6MfFBJoeYX3zvd96 WjgA== X-Gm-Message-State: AOAM533wH7zcBiEyIvAtMY5uXbms+Ds2aId4y2TvF7SWthxG90GYLZPk X1WbLvwxCjOfwdMTFP9hCXBbU2Ih0zXseA== X-Google-Smtp-Source: ABdhPJz22S7ypactvLndeWRbOXsV+ju4YNfrvQbvwmobpFkOHHbBFUpNw4+r+2Qh9wlROzlHqrPFsQ== X-Received: by 2002:a17:90a:f314:: with SMTP id ca20mr20376245pjb.210.1629723038167; Mon, 23 Aug 2021 05:50:38 -0700 (PDT) Received: from ?IPv6:2804:431:c7ca:cd83:c38b:b50d:5d9a:43d4? ([2804:431:c7ca:cd83:c38b:b50d:5d9a:43d4]) by smtp.gmail.com with ESMTPSA id j68sm19512021pgc.44.2021.08.23.05.50.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 23 Aug 2021 05:50:37 -0700 (PDT) Subject: Re: [PATCH/2nd version] Re: nextafter() about an order of magnitude slower than trivial implementation To: Stefan Kanthak , Joseph Myers Cc: Szabolcs Nagy , libc-help@sourceware.org, libc-alpha@sourceware.org References: <20210818125119.GH25257@arm.com> <741b2c48-a754-51c5-cb72-a2f97795e30f@linaro.org> <2246A49E9E024DDBB9AF9C212B29712A@H270> From: Adhemerval Zanella Message-ID: <3bd22d9b-dd89-d5f1-c282-3e5927f991d1@linaro.org> Date: Mon, 23 Aug 2021 09:50:35 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <2246A49E9E024DDBB9AF9C212B29712A@H270> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, 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: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Aug 2021 12:50:49 -0000 On 20/08/2021 17:19, Stefan Kanthak wrote: > "Joseph Myers" wrote: > >> On Fri, 20 Aug 2021, Stefan Kanthak wrote: >> >>> if(ax > 0x7ffull << 53) /* x is nan */ >>> return x; >>> if(ay > 0x7ffull << 53) /* y is nan */ >>> return y; >> >> How has this been tested? I'd have expected it to fail the nextafter >> tests in the glibc testsuite (libm-test-nextafter.inc), because they >> verify that sNaN arguments produce a qNaN result with the "invalid" >> exception raised, and this looks like it would just return an sNaN >> argument unchanged. > > It doesn't look like it would, it REALLY does! > As I explicitly wrote, my changes avoid FP operations if possible. > > > > | If x or y is NaN, a NaN shall be returned. > > I choose to return the argument NaN, what both POSIX and ISO C allow. > If my mind serves me well, one of the former editions even stated > "If an argument is NaN, this argument shall be returned". This may > but be influenced/spoiled by > > > | If either x or y is a NAN, then the return value is one of the input NANs. > > > If that bothers you, change these 4 lines to either > > if(ax > 0x7ffull << 53) /* x is nan */ > return 0.0 + x; > if(ay > 0x7ffull << 53) /* y is nan */ > return 0.0 + y; > > (the addition has eventually to be guarded from optimisation) or > > if(ax > 0x7ffull << 53 /* x is nan */ > || ay > 0x7ffull << 53) /* y is nan */ > return x + y; Sorry, but sending half-baked patches where one will need to evaluate and decide which is the best strategy is not the best way forward to accept this change. Please do follow the contributor checklist [1] and the suggestion I gave you when you asked for input in the libc-help: * Check for regressions by testing your patch against glibc testsuite (make check). As Joseph has put, your patch triggers a lot of regressions: $ grep ^FAIL math/subdir-tests.sum FAIL: math/bug-nextafter FAIL: math/test-double-nextafter FAIL: math/test-float32x-nextafter FAIL: math/test-float64-nextafter FAIL: math/test-misc * You removed the copyright since it is a new implementation, but the any code still requires a Copyright (even if is not assigned to FSF). Check the item 3.4 on the Contributor checklist. * Also, since is a new implementation follow the usual style and code guidelines instead of using the current one used. * It would be good to add a benchmark to evaluate this change if the motivation for your change is performance. Follow what other math benchmarks does on benchtests (exp2 or exp10 for instance). * Sending a patch with a define switch and stating "Choose whatever you like best" does not help in anything and put the burden or evaluating your change to the maintainers. [1] https://sourceware.org/glibc/wiki/Contribution%20checklist