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: Export of FOR (and FORUNTIL) assumes ExprOrNil for OFOR.Right, but it is StmtOrNil #25222

Closed
dr2chase opened this issue May 2, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@dr2chase
Copy link
Contributor

commented May 2, 2018

What version of Go are you using (go version)?

1.11dev

What did you do?

I enabled inlining for OFOR nodes, which caused the export of an OFOR node, which failed.

<autogenerated>:1: internal compiler error: cannot export = (20) node
==> please file an issue and assign to gri@

goroutine 1 [running]:
runtime/debug.Stack(0x0, 0x0, 0x0)
	/usr/local/google/home/drchase/work/go-ssa/src/runtime/debug/stack.go:24 +0xa7
cmd/compile/internal/gc.Fatalf(0xbe0f4c, 0x47, 0xc0068091f8, 0x2, 0x2)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/subr.go:182 +0x1f7
cmd/compile/internal/gc.(*exportWriter).expr(0xc0059fa900, 0xc00219e900)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:1319 +0x2a0
cmd/compile/internal/gc.(*exportWriter).exprsOrNil(0xc0059fa900, 0xc00219e880, 0xc00219e900)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:1341 +0x8d
cmd/compile/internal/gc.(*exportWriter).stmt(0xc0059fa900, 0xc00219e800)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:1053 +0x544
cmd/compile/internal/gc.(*exportWriter).stmtList(0xc0059fa900, 0xc006809318)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:974 +0xa6
cmd/compile/internal/gc.(*iexporter).doInline(0xc005f1c000, 0xc00046f1d0)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:505 +0xb4
cmd/compile/internal/gc.(*exportWriter).funcExt(0xc0059fa870, 0xc00046f1d0)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:953 +0x144
cmd/compile/internal/gc.(*iexporter).doDecl(0xc005f1c000, 0xc00046f1d0)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:434 +0x136
cmd/compile/internal/gc.iexport(0xc0055a2000)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:276 +0x38d
cmd/compile/internal/gc.dumpexport(0xc004606000)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/export.go:79 +0x8f
cmd/compile/internal/gc.dumpCompilerObj(0xc004606000)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/obj.go:119 +0x39
cmd/compile/internal/gc.dumpobj1(0x7fffbe08a3cf, 0x23, 0x3)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/obj.go:70 +0x15b
cmd/compile/internal/gc.dumpobj()
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/obj.go:51 +0x60
cmd/compile/internal/gc.Main(0xbe3d58)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/main.go:684 +0x277c
main.main()
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/main.go:49 +0x96

Root cause is

	case OFOR:
...
		w.exprsOrNil(n.Left, n.Right) 
...

because n.Right is the increment of the for loop:

	OFOR      // for Ninit; Left; Right { Nbody }

i.e., n.Right is a statement, not an expression, and may be nil.

@dr2chase

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2018

@gri, @mdempsky could you give this a look, if it is easy and low risk it would help with inlining for loops in 1.11, requested in #14768.

@andybons andybons changed the title Export of FOR (and FORUNTIL) assumes ExprOrNil for OFOR.Right, but it is StmtOrNil cmd/compile: Export of FOR (and FORUNTIL) assumes ExprOrNil for OFOR.Right, but it is StmtOrNil May 2, 2018

@andybons andybons added this to the Go1.11 milestone May 2, 2018

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2018

This shouldn't be too hard; the current code simply makes a wrong assumption for OFOR.

@dr2chase Do you have a complete reproducible example for me so I can test this?

@dr2chase

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2018

Requires a slightly tweaked compiler so it will attempt to inline OFOR, will put together that CL. I'll just reconnect all the inlining knobs, and add one for OFOR, we'll need this anyway to work on heuristics.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 14, 2018

Change https://golang.org/cl/118996 mentions this issue: cmd/compile: add some inliner knobs for parameter search

@dr2chase

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2018

There you go, a new CL for inlining knobs, plus a reproducer (except this one complains about a different node, 26):

cat > main.go <<EOF
package main

func Foo(x int) int {
	a := 0
	for i := 0; i < x; i++ {
		a += a*a + i
	}
	return a
}

func main() {
	for i := 0; i < 7; i++ {
		println("Foo(", i, ")=", Foo(i))
	}
}
EOF
go build -gcflags -d=env=INLLOG=1:INLFOREXTRA=1 main.go

With this set of flags, it fails with the original error (node 20) somewhere compiling runtime

go build -gcflags all=-d=env=INLLOG=1:INLFOREXTRA=1 main.go
@gopherbot

This comment has been minimized.

Copy link

commented Jun 18, 2018

Change https://golang.org/cl/119595 mentions this issue: cmd/compile: fix exporting of 'for' loops

@gopherbot gopherbot closed this in f125052 Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.