This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PING][PATCHv3 1/2] aarch64: Hoist ZVA check out of the memset function
On Thursday 12 October 2017 07:31 PM, Szabolcs Nagy wrote:
> there was no testing on real workloads only on
> a ubenchmark that has issues outlined by wilco.
It is the kind of change that shouldn't even need a benchmark. It is a
no-brainer - I am hoisting out a check that is done inside every
function call into the IFUNC, thus reducing a branch inside the code.
Compilers do this all the time for loop invariant ops and this is no
different.
I have split out the discussion about the benchmark because I believe
Wilco has misread the benchmark and I've tried to explain how in that
thread.
> it is known that some cores are sensitive to the
> exact code sequence in memset and you changed it
> without testing the affected cores (cortex-a53),
> probably only wilco knows why he wrote the code
> exactly the way it is written, so i need evidence
> that there is no regression or wilco's approval.
I've posted two patches and it looks like you're conflating the two.
The second patch needs more testing I agree but we're discussing the
first patch, which is far more straightforward.
> mustang is a devboard with x-gene cores, which is
> not representative of the existing aarch64 cores
> in use, but even on mustang it's not clear that the
> ubenchmark speed up mean improvement in practice.
> (string functions are hard to benchmark, there are
> many cases and concerns, which is why i'm conservative
> about accepting patches to them)
>
> in principle the approach is fine, but it will take
> more time to get confident about the patch.
Like I said before, the benchmark has been misunderstood and I maintain
that the gains will be seen in every benchmark for the input range I
pointed out without causing any noticeable change elsewhere.
So barring those things I read your response as you thinking that the
patch is OK but you're afraid of some unknown unknowns hitting back. In
that case I suggest we commit this now, let CI loops over take over (in
Cavium, Linaro, ARM, etc.) and then point out regressions if any. The
valid patch lying around on the mailing list or an arbitrary branch is
never going to get tested.
Siddhesh