Re: [PATCH] Improves __ieee754_exp() performance by 2x-5x on sparc/x86.

[Joseph Myers <joseph at codesourcery dot com>]

On Wed, 3 Jan 2018, Patrick McGehearty wrote:

> I have not been updating any of the earlier comments when I responded
> to the later comments. I just added the newest comment to the top of
> the earlier patch comments to allow easy access to this PATCH history.
> If we need another patch, I'll rework the comments as you suggest.

The entirety of any proposed commit is subject to review.  This includes 
the commit message as well as the patch itself.  That means it must be 
unambiguous exactly what the proposed commit message is - and that message 
must all be in one place, not needing composing from pieces in multiple 
messages, and must correspond to the patch sent with that message.  Again, 
I recommend the same format used by the Linux kernel, of having the commit 
message first, with anything about changes between different patch 
versions afterwards, following a "---" line.  That's also what "git am" 
expects (though we can't quite use git am for glibc because of the need to 
update the ChangeLog file at commit time).

> I'm not clear about how "make regen-ulps" is part of the patch process.
> I've not seen any discussion of that before.
> Do I need to include the changes to the ulp test data as part
> of the patch? I had assumed that was handled as a separate
> patch after the code changes were merged.

Any patch submission should have non-regressing test results, on at least 
one platform that is explicitly named in the commit message as a platform 
on which the patch was tested.

This means that if a patch needs ulps updates to avoid FAILs of math/ 
tests, those ulps updates (on at least one named platform used for 
testing) must be included in the patch, and the patch as posted, complete 
with the ulps updates, must have been tested without resulting 
regressions.  It should be obvious from the problems with the previous 
patch version that it's *not* sufficient simply to eyeball the logs for 
failing tests - you need actually to update ulps and to confirm that FAILs 
don't appear given the ulps updates, because there are other ways those 
tests can fail that are not fixed by such updates.

If you test on more than one platform, including the ulps updates for all 
those platforms in the patch is a good idea.

> Also, when I run "make check" on the base code for Sparc, I'm getting
> some additional ulp failures for cpow, ctan, ctanh and a few others.
> These did not occur with my 8th patch submission but did with my
> 9th patch. They do not occur on the x86 tests.
> These ulp failures are unchanged by my exp() code, so I've
> concluded they are not related to my work.

If there are preexisting ulps failures, that means you need to send two 
patches.  The first one would be the results of "make regen-ulps" for 
current glibc, to fix the preexisting failures (and such a patch could be 
committed as obvious without prior review).  The second one would be 
relative to a tree with the first one applied, and would include whatever 
further ulps updates the exp patch requires.

Architecture maintainers should update ulps files as needed during the 
release freeze, and they or others may also do updates at other times as 
and when convenient.

-- 
Joseph S. Myers
joseph@codesourcery.com