Skip to content
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: internal compiler error: schedule does not include all values in block on s390x #39472

Closed
ALTree opened this issue Jun 9, 2020 · 6 comments

Comments

@ALTree
Copy link
Member

@ALTree ALTree commented Jun 9, 2020

$ gotip version
go version devel +cacac8bdc5 Tue Jun 9 02:49:06 2020 +0000 linux/amd64

The following program:

package p

func f() bool {
	var c float64
	c = 0
	c = -c + 1
	return (c != 0) == (c != 0)
}

Crashes the tip compiler when built for s390x with optimizations disabled:

$ GOARCH=s390x gotip tool compile -N crash.go

crash.go:7:27: internal compiler error: 'f': schedule does not include all values in block b1
goroutine 1 [running]:
runtime/debug.Stack(0xd4bb80, 0xc00000e018, 0x0)
	/home/alberto/go/src/runtime/debug/stack.go:24 +0x9f
cmd/compile/internal/gc.Fatalf(0xc00001a300, 0x36, 0xc0003cb920, 0x2, 0x2)
	/home/alberto/go/src/cmd/compile/internal/gc/subr.go:199 +0x1b0
cmd/compile/internal/gc.(*ssafn).Fatalf(0xc000011d10, 0x71b100000002, 0xc63c80, 0x30, 0xc000046b10, 0x1, 0x1)
	/home/alberto/go/src/cmd/compile/internal/gc/ssa.go:6844 +0x1a5
cmd/compile/internal/ssa.(*Func).Fatalf(...)
	/home/alberto/go/src/cmd/compile/internal/ssa/func.go:625
cmd/compile/internal/ssa.schedule(0xc00013c420)
	/home/alberto/go/src/cmd/compile/internal/ssa/schedule.go:306 +0x1baa
cmd/compile/internal/ssa.Compile(0xc00013c420)
	/home/alberto/go/src/cmd/compile/internal/ssa/compile.go:93 +0x9d1
cmd/compile/internal/gc.buildssa(0xc00013c2c0, 0x0, 0x0)
	/home/alberto/go/src/cmd/compile/internal/gc/ssa.go:460 +0xd25
cmd/compile/internal/gc.compileSSA(0xc00013c2c0, 0x0)
	/home/alberto/go/src/cmd/compile/internal/gc/pgen.go:317 +0x5d
cmd/compile/internal/gc.compile(0xc00013c2c0)
	/home/alberto/go/src/cmd/compile/internal/gc/pgen.go:275 +0x345
cmd/compile/internal/gc.funccompile(0xc00013c2c0)
	/home/alberto/go/src/cmd/compile/internal/gc/pgen.go:220 +0xc5
cmd/compile/internal/gc.Main(0xc6e560)
	/home/alberto/go/src/cmd/compile/internal/gc/main.go:747 +0x344f
main.main()
	/home/alberto/go/src/cmd/compile/main.go:52 +0xb1

It compiles fine on go1.14.4, so this is a tip regression. I'm tentatively labelling this as a release-blocker, but leaving to you to decide if this is fine.

cc @mundaym @randall77 @cherrymui

@ALTree ALTree added this to the Go1.15 milestone Jun 9, 2020
@marigonzes
Copy link

@marigonzes marigonzes commented Jun 9, 2020

Bisection points to b2790a2

@mundaym
Copy link
Member

@mundaym mundaym commented Jun 9, 2020

Slightly more minimal reproducer:

package p

func f(x float64) bool {
	x += 1
	return (x != 0) == (x != 0)
}

The problem is that the s390x rules can insert duplicate Select0 and Select1 ops. For example, these rules (added for Go 1.15 in b2790a2) insert Select1 ops:

(LTDBR (Select0 x:(F(ADD|SUB) _ _)))   && b == x.Block -> (Select1 x)
(LTEBR (Select0 x:(F(ADDS|SUBS) _ _))) && b == x.Block -> (Select1 x)

That isn't a problem with CSE enabled because it will de-duplicate them. One possibility is that we pull the Select0 and Select1 de-duplication code that currently exists in CSE into a separate, mandatory, pass. I'll send that as a CL.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 9, 2020

Change https://golang.org/cl/237118 mentions this issue: cmd/compile: always tighten and de-duplicate tuple selectors

@beoran
Copy link

@beoran beoran commented Jun 9, 2020

@mundaym As you stated "The problem is that the s390x rules can insert duplicate Select0 and Select1 ops.". I may be out of line here, but I'd say the right fix is to avoid doing that in the first place, not add another pass that cleans up the mistake from the previous pass.

@mundaym
Copy link
Member

@mundaym mundaym commented Jun 9, 2020

@beoran Yeah, that is one possibility. Though I would point out that there is no good way to have tuple optimization rules without encountering the duplication issue and it may well be hit again in the future.

The main advantage of doing the cleanup as a separate pass is however that it means that none of the passes up until that point have to worry about maintaining this invariant, whether it be CSE, optimization rules or some other future optimization pass that has yet to be added. That's quite nice since every invariant like this that we have to maintain is yet more complexity in individual passes. There is no real reason to treat selectors differently from other ops in most passes, only the scheduler & register allocator actually care.

@beoran
Copy link

@beoran beoran commented Jun 10, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.