cmd/compile: possible confusion over whether phis are always the first values in a block

[josharian]

At some point in the distant past, it was (almost) always the case that OpPhi values, if present, were the first values in the block. And some passes appear to assume that. Here are two comments from the deadstore pass:

		for _, v := range b.Values {
			if v.Op == OpPhi {
				// Ignore phis - they will always be first and can't be eliminated
				continue
			}

/* ... */

		// walk to previous store
		if v.Op == OpPhi {
			continue // At start of block.  Move on to next block.
		}

But it is definitely no longer the case that all phis are the at the beginning of the block. Among other reasons, the nilcheck and writebarrier passes call storeOrder, which re-orders values in a way that would move some phis away from the beginning.

I tried to test this by adding code that randomizes value order before all passes prior to scheduling and running toolstash, but register selection changes made that unhelpful before I could detect anything substantive. Sample:

inconsistent log line:
/var/folders/1t/n61cbvls5bl293bbb0zyypqw0000gn/T/go-build711603550/runtime.a.log:30617:
	obj: 00154 (/var/folders/1t/n61cbvls5bl293bbb0zyypqw0000gn/T/toolstash-check-240465411/go/src/runtime/mbitmap.go:1533)	ANDQ	R14, R11
/var/folders/1t/n61cbvls5bl293bbb0zyypqw0000gn/T/go-build711603550/runtime.a.stash.log:30617:
	obj: 00154 (/var/folders/1t/n61cbvls5bl293bbb0zyypqw0000gn/T/toolstash-check-240465411/go/src/runtime/mbitmap.go:1533)	ANDQ	R11, R14
2017/04/29 14:53:10 exit status 2

Let's:

• decide if/when we want to assume that OpPhis are always first, and if we ever do, add an ssa internal check for it
• fix up any code that depends on faulty assumptions
• make the schedule pass more fully deterministic
• do some randomization-based testing to make sure that any assumptions about value order are safe/correct (perhaps always when ssa/check is on, somehow?)

cc @randall77 @cherrymui @dr2chase

[cherrymui]

I thought storeOrder doesn't move Phi from the beginning. I'll check. (EDIT: this is not true)
Do you have a testcase that demonstrates it?

[cherrymui]

Keith has a CL https://go-review.googlesource.com/c/33909/ that does the value order randomization.

[cherrymui]

@josharian you are right that storeOrder doesn't always keep Phis at the beginning. And there are other passes that doesn't keep this. For example, decompose may decompose a Phi into multiple Phis and add them to the end. Maybe we should make code don't depend on this. Many passes indeed don't need this.

[josharian]

Maybe we should make code don't depend on this.

SGTM

[josharian]

And in passes where this proves complicated or risky or expensive, we can just fix it up at the beginning. That's a fairly cheap O(1) single pass through the slice.

[dr2chase]

The loop-unroller also appends new phis to the per-block value slices.

[josharian]

A visual inspection of the ssa passes suggests that the only two passes that depend on phis being first are deadstore and the various rule-based rewrites (see e.g. comments in canMergeLoad).

[gopherbot]

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

[randall77]

Before values are ordered in a block, phis will not necessarily be first. After they are ordered (in the schedule pass), phis must be first. I mailed CL 45018 to check the latter in check.go.

CL 33909 (randomize the order of b.Values before the schedule pass) will not get in for 1.9. I intend to get that in for 1.10. It will help make sure that before the schedule pass, we don't depend on phis being first.

I looked closely at both deadstore and canMergeLoad - they don't depend on phis being first in b.Values, just being logically the first store in the store chain for a block. That should still be the case, even if they are out of order in b.Values.

[gopherbot]

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