New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmd/compile: ssa: LoweredNilCheck downgrade perfomance of binary search in array #30366
Comments
Just from looking at the assembly here on my phone, I’d say that that line is checking whether p is nil; it’d fault if so. It is thus not redundant. If that’s right, of course, the nil check should be hoisted out of the loop. And I believe that @randall77 had plans to mark some manually constructed unsafe.Pointers as non-nil (which this one clearly is, as it points to the first element of a slice). |
Yeah, this is probably true. But pay attention that there is no "complete" check, it's only TEST instruction. No jumps, no any other side effects which should follow after it. |
If it faults, we’ll receive a segfault signal, which the runtime will catch and turn into a panic. |
Why is this test inside the loop? Pointer is declared at the beginning of the function. |
Yes, as I wrote:
|
It seems to me, this test is not for null pointer. This test check Please see full asm code:
Please correct me. |
I am wrong. |
In case it helps you understand, this is how the compiler views that code, step by step: (Generate with |
Thanks. Then, AX = j, SI = h, BX = i, DX = pointer to data. |
We found |
But we need to pull this check out of the loop |
I analyze this report and see:
At the "early copyelim" stage, v32 is not reduced. Another reason for remove this check: NilCheck does not add protection against a null pointer, since then there is "v38 (11) = PtrIndex <*uint32> v30 v33; v39 (11) = Copy v31; v40 (11) = Load v38 v39", which can also trigger SEGFAULT on invalid pointer. |
CC @martisch |
My change only marks the results of unsafe pointer arithmetic as non-nil. It doesn't do so for simple casts. We could mark the results of |
Change https://golang.org/cl/163740 mentions this issue: |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
Create a binary search in an sorted array int32 without checking boundaries, using unsafe.
What did you expect to see?
Maximum performance of compiled code without checking the boundaries inside the loop.
What did you see instead?
Compiler hidden error is possible, that downgrade perfomance, which may occur in other cases.
The result of compiling the above code:
The compiler creates an additional instruction
TESTB AL, (DX)
for comparing two numbers, the result of which is not used further, because is replaced by the nextCMPL DI, CX
.The text was updated successfully, but these errors were encountered: