cmd/compile: instructions being lifted before arch support tests

[randall77]

package main

import "math/bits"

func f(x uint64) int {
	r := 0
	for range 5 {
		r += bits.OnesCount64(x)
	}
	return r
}

GOAMD64=v1 go build -gcflags=-S test.go

In the assembly output, the POPCNTQ instruction has been lifted to before the start of the loop. We can't do that, unless we also lift the x86HasPOPCNT test that wraps POPCNTQ instructions in v1 mode.

This is a consequence of https://go-review.googlesource.com/c/go/+/697235 . Maybe we should revert it. (It was also responsible for #78892. I'm not sure what else might be lurking.)

[Jorropo]

I think we need something like faultOnNilArg0 but for CPUID bits.

[gabyhelp]

Related Issues

• cmd/compile: runtime check for availability of popcount is spammy #30946
• dev.boringcrypto: TestGoAMD64v1 broken at HEAD #49402 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[randall77]

I think we need something like faultOnNilArg0 but for CPUID bits.

That's a possibility. I was thinking it might be safer to go in the other direction - mark instructions we know are safe to lift. That might be more verbose though.

[randall77]

This issue reveals a pretty fundamental flaw in our SSA representation. There's no ordering edge between a test that an instruction is ok to use, and the instruction itself. It would be really nice if there was an SSA edge that represented that ordering somehow.
We have a similar problem with nil checks, which we kind of hack around. It would be nice if there was an SSA edge from the nil check to the load/store that requires it.

Not sure how to do that.

[Jorropo]

In an other issue I wanted to add nilcheck to the mem chain but this has other negative downsides.

[prattmic]

cc @golang/compiler

[Jorropo]

I was working on a patch to lift up all possible instructions from the likely edge of a branch to before branche even if they are only used in the likely branch (basically pulls the whole graph up until the first mem producer or phi node and any of it's transient consumer).
But this has the same issue.

[prattmic]

This is a consequence of https://go-review.googlesource.com/c/go/+/697235

This is new in 1.27, so I'm tentatively marking this as a release blocker while we decide what to do.