From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1033.google.com (mail-pj1-x1033.google.com [IPv6:2607:f8b0:4864:20::1033]) by sourceware.org (Postfix) with ESMTPS id 3E6B0385842C for ; Mon, 23 Aug 2021 12:50:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3E6B0385842C Received: by mail-pj1-x1033.google.com with SMTP id n13-20020a17090a4e0d00b0017946980d8dso18545272pjh.5 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=OCXJvv913kAkSkY28o1CA9TukUT29SnrFfhLKnHf/Qqdz+BD4EM6LBNkmyQ1IemiQM LN2559+jaF1wZnkgp/8B+9R7ZWEgI4JAyEVYwspjaAogVthjUDmp9fgMt/8mwRw5L8XR EUG8+fImdVwD5WU5E6e8o5wiAxF12reiviG4M6yAJ/J1bO9ZnX51lj9j96OxMKS55Rlc ktGsz174tWTZDytu+QOLBwzd0Gd8qT81ZY8BGWD4OkpJgjLq9Eqs0BU4ScyFj3FMFrS9 Pr2UuV3CpWg/wIIm9Wyx4RUWNj2NAYV/sJ2Cp42O9+o8p696klYMqLmEXMuzVUHgtDYz XZYA== X-Gm-Message-State: AOAM533DCfZKUIWk/skCn+Gf2DIrewUMmItGoyEq1RB6t2Bptc95fyOq nQCix145YU3Ps5kSsK5Ezngaww== 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-help@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-help mailing list List-Unsubscribe: , List-Archive: 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