/cc @randall77

Amateur response:

The JMP at cd9 goes to cf8, which JMPs at cff to cdb. So all the jumps are used, and that's OK. Is this compiler output or final generated code, because I would have expected the linker to do some branch folding if this is the final output.

I believe the TESTLs are nil checks introduced by the compiler as guarantees that indirections through nil pointers cannot be used to access random memory.

The "register spill" is putting a value back on the stack. It may be unnecessary but it's hard to tell from this incomplete fragment.

To me, this all looks pretty normal.

(this is not a bug report, but a missing optimization issue, for future consideration)

The code shown is the final, complete code of Search(), dumped with go tool objdump. It's not partial, it's the full assembly output for that function.

The jumps are all used but they are inefficient compared to the minimal possible output.

The TESTL might be residual of removed nil-checks (as the flags set by those tests are not checked afterwards), but if so, they should removed as well.

The register spill is in fact used later on at 0x4096cea, but there are a dozen of registers available so there's really no reason for spilling to the stack.

I may be wrong but I'm pretty sure the TESTL remains there for the side effect of validating the struct's base address before indirecting through to the field. That is the nil check.

I am surprised that the linker didn't fold the branches.

@brtzsnr

radix.go:29 0x4096ca9 8401 TESTL AL, 0(CX) Is a nil check. It'll fail if CX == 0 (in the code this is t).

radix.go:32 0x4096cc2 48891c24 MOVQ BX, 0(SP) Is another case for suboptimal register allocator. Here are a few more examples: #14504.

radix.go:39 0x4096ccf 488d05cad11f00 LEAQ 0x1fd1ca(IP), AX happens because the compiler doesn't do loop hoisting. I would love to have it for https://go-review.googlesource.com/#/c/21434/, too.

radix.go:36 0x4096cd9 751d JNE 0x4096cf8 second jump happens because we don't merge consecutive ifs on the same variable. Same happens here.

func g(a bool) int {
    x := 1
    if a {
        x = 2
    }
    if a {
        x += 1
    } else {
        x += 2
    }
    return x
}

-Correction-: radix.go:36 0x4096cd9 751d JNE 0x4096cf8 happens because of the following. Will fix later this evening.

    v40 = Copy <bool> v35
    If v40 -> b14 b13

Sorry if I'm dense (I'm often confused by the ASM syntax you use, I'm used to a different syntax). TESTL just sets the flags; who's checking for those flags? Who's handling the result of such nilcheck? I don't see this happening in the above code.

TESTL AL, 0(CX) TESTL tests for flags but in this case it reads from 0(CX). If CX == 0 (t == nil) then 0(CX) == 0 which generates a page fault which is transformed by the runtime in a panic.

Thanks for clarifying, I didn't know nilchecks relied on page faults. Makes sense now.

I stand corrected, again:

    v40 = Copy <bool> v35
    If v40 -> b14 b13

was not the cause of this. It's just the lack for merging consecutive if's on the same variable. This is probably a common case where we would benefit from merging consecutive if's.

Loop hoisting, we could do. I got as far as writing the detector, measured hits, decided that there weren't enough to get that excited. But I could revive that, if people are interested.

Algorithm was:

1. detect (reducible) loops
2. determine articulation blocks (those which must execute if loop executes)
3. find fixed point of "values not relying on within-loop values"
4. filter out the ones that are too simple to care about

Only thing remaining is to move those into the predecessor of the loop header (assuming after critical edges eliminated). Note that this increases register pressure, so you might want a heuristic on loop size.

On "merging consecutive ifs", see also #12628.

The unnecessary spill has been fixed (not sure when). I just sent out a CL to remove the redundant zero extension.

The constant hoisting is #15808
The redundant branch is #12628

I'll close this in deference to those bugs.

CL https://golang.org/cl/28191 mentions this issue.

@randall77 the CL has been forgotten. Do you plan to submit it?

Yes, I just need to ping a reviewer. It will get in for 1.8.